Skip to content

Conversation

@carloalbertobarbano
Copy link
Member

Changelog:

  • Add proper support for multiple backends (numpy, torch, tensorflow)
  • Add installation flavors in setup.py

@carloalbertobarbano
Copy link
Member Author

I made a RC release v1.2.0-rc1. If everything is okay, I will make a final release and update the pypi package.

@carloalbertobarbano carloalbertobarbano added release PR for new release and removed enhancement New feature or request labels Aug 9, 2022
@andreped
Copy link
Collaborator

andreped commented Aug 9, 2022

I made a RC release v1.2.0-rc1. If everything is okay, I will make a final release and update the pypi package.

I can take a look now. Will make a PR if I see something crucial.

@andreped andreped self-requested a review August 9, 2022 10:07
Copy link
Collaborator

@andreped andreped left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments:

  • Why were the macOS tests removed?
  • Run tests on ubuntu-18.04, as software is forward-compatible only on ubuntu. If it works on ubuntu 18, it works on 20/22, but not certain the other way.
  • Why was Python 3.6 dropped? I would think there are still some people using it, but perhaps newer versions of TF don't have precompiled wheels for 3.6 anymore? If we drop 3.6, we must update this in setup.py and remove this.
  • This comment for tf percentile is for pytorch. I guess we could rewrite it slightly, but not critical.
  • This description also needs updating to match that we support all backends. Update the About in accordance with the Issue #20, and use the new about there.
  • Lastly, remember to upload the generated wheel to the github release tag as well, such as for this tag.

Please, adjust for these comments before merge with main.

@andreped
Copy link
Collaborator

andreped commented Aug 9, 2022

Just saw that the TF test failed with Python 3.6, but that is only because newer versions of TF weren't available at the time. You could remove Python 3.6 from the tests, or add specific versions of TF in that test.

In a related project, I did this, if you wish to keep the Python 3.6 test.

@carloalbertobarbano
Copy link
Member Author

I really feel like 164 tests are a bit too many..
I would restrict testing to ubuntu only. We do not make use of system calls and pytorch, tf and numpy should be already tested on all systems.

@andreped
Copy link
Collaborator

andreped commented Aug 9, 2022

I really feel like 164 tests are a bit too many..

I believe we should perform these tests before we make a release, but it is not necessary for every single commit.

I would restrict testing to ubuntu only.

We could do this for each commit. It is also likely not necessary to test multiple different tf/pytorch versions for each commit.

Otherwise, I think the fork is ready to be merged, especially as the last CI ran successfully (see here)


EDIT: We can make an issue on this, and look at this some other time, but I would not spent a lot more time on that, as it is not crucial for this release.

@carloalbertobarbano carloalbertobarbano merged commit 42561ce into main Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release PR for new release

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants