-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for pytorch 1.5.0 #122
Conversation
Currently conda dry runs for retrieving cache hashes are failing, because the session is not defined in Nox for 1.5.0. You need to add "1.5.0" to PYTORCH_VERSIONS in gen_github_actions.py, that's the single source of truth that Nox adheres too. |
Both Windows and Ubuntu are failing in ways I currently don't understand. Can you try disabling extension building with ninja? Add a use_ninja=False flag to the options here Line 180 in 24d0275
|
That is one solution, another one proposed in the changelog is the following: I will try both and see which one is better (faster, easier to configure, and everything). |
Edit: I was mistaken, we are using the correct version. |
Now everything works as expected, and the errors on MacOS are unrelated to brevitas-support for 1.5 |
setup.py
Outdated
MIN_TORCH_JITTABLE_VERSION = "1.3.0" | ||
MAX_TORCH_JITTABLE_VERSION = "1.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these two constants into brevitas/__init__.py where they are also used, so that we have a single source of truth. init.py is not importing anything new compared to setup.py so it should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for coherence it should be MAX_TORCH_JITTABLE_VERSION = 1.4.0, since the MIN one is inclusive, and then adjust comparison operators in init and setup accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In init.py there's the import of packaging that breaks everything. Considering the workaround we have in setup.py for having it installed automatically, I would prefer duplicating the two constants for now and look for a cleaner solution in a second moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just move the import inside the run() methods and then pass the value to the template function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we can just define a bool is_ste_jittable in __init__.py and then import it into setup.py from within the run methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing within the run methods causes all the code in init to be executed, included the search for the "_C" file. But at setup-time, the _C module has yet to be created, so it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, let's put the bool in brevitas.config and import it into both init and setup. We need to setup a separate pull to move docstrings out of config first though, and into brevitas' init, or we will have another setup time import error.
Also please rebase into two commits, one "Setup: add support for Pytorch 1.5.0" with setup.py and init.py, the other "CI/CD: run CI against Pytorch 1.5.0" with everything else |
4f00410
to
9ca2e31
Compare
Please rebase after #136 . |
No fix seems to be working for MacOS, pytorch1.5 and python 3.6. |
As per #119
Initial commit, more to follow.