-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Support python3.10 #1137
Support python3.10 #1137
Conversation
* bumps torch to 1.10 * bumps torchvision to latest * uses onnxruntime 1.12 on py 3.10 (currently in prerelease: microsoft/onnxruntime#9782) which isn't ideal, so probably this isn't mergeable until https://github.com/microsoft/onnxruntime/projects/9 is done.
torch doesn't support python 3.10 in older versions.
Pull Request Test Coverage Report for Build 2813198870
💛 - Coveralls |
I removed the specific version requirement for |
🍳 eggggggs-cellent. That's good news. Thanks for doing the leg-work @konantian :) Now we just have to wait. |
This is the first onnxruntime to support Python 3.10.
I've updated this given that the latest onnxruntime is out with py310 support. This is ready for review! 🤞 |
I installed ivadomed with conda (python=3.10) on NeuroPoly's The short test summary info is:
I then ran the individual tests that failed:
|
Thank you for the very nice bug report @shuaisong9! It's weird that it failed there but not on GitHub. I'll have to look into it. Though then again I'm not surprised, it's something to do with the GPUs, which we don't test regularly (because GitHub doesn't offer them):
|
I tried the same test on Python 3.9 with
versions
I got the exact same error:
|
Oh and fail4 is because those tests were run on
|
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.
I did a sanity check on my end using GPU (Compute Capability 7.6). Just an observation that would be nice to confirm on the lab GPUs as well: torch<1.11.0+cu111
doesn't complete the test in an hour, but torch<1.11.0+cu102
completes it in about 2 min. I believe this is related to #1141 which seems to be fixed in torch==1.11.0
release as torch==1.11.0+cu111
works great.
onnxruntime>=1.12 is needed for python310, and is available for python39, python38 and python37 _from PyPI_. But it's not available for all those versions on ComputeCanada's repo (https://docs.alliancecan.ca/wiki/Available_Python_wheels). So onnxruntime>=1.8 should be equally functional and more broadly compatible.
Co-authored-by: Kanishk Kalra <36276423+kanishk16@users.noreply.github.com>
@kanishk16 what do you think now? I admit don't really understand python versioning. There's too many operators. I don't know when it's best to use which, if I should use |
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.
I admit don't really understand python versioning. There's too many operators. I don't know when it's best to use which, if I should use ~=M.m.p or ~=M.m or >=M.m or what.
I couldn't agree more, but reading up a bit on this, especially https://bernat.tech/posts/version-numbers/ and https://snarky.ca/why-i-dont-like-semver/ convinced me about not having an upper cap unless absolutely necessary. IMO, it boils down to how much we would like to restrict ourselves as ~=M.m.p < ~=M.m < >=M.m
(in the order of most to least restricting). Also, it depends upon the maturity as well as the release cycle of the library/package per se. It is highly unlikely for torch to make a major release soon. Similarly, for onnxruntime. We should be good with either ~=M.m or >=M.m
as both are pretty mature library/package in this case.
Hopefully as the ecosystems evolve and our experience grows we'd be able to decide better. What do you think @kousu?
torch>=1.8.1,<=1.11.0 | ||
torchvision>=0.9.1,<=0.12.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.
torch>=1.8.1,<=1.11.0 | |
torchvision>=0.9.1,<=0.12.0 | |
torch>=1.8.1 | |
torchvision>=0.9.1 |
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.
Removing the upper bound on the version exposes us to potential issues if some breaking changes versions are made in future versions of either packages, isn't it? I would only keep the versions that we did tested (i.e. restricting to these with >=min, <=max
bounds).
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.
Absolutely... In hindsight, we're already making a big transition and I guess it would be better if we have an upper bound to narrow down in case we find any unusual behaviour.
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.
But are you saying we should test every version of every dependency implied by our >=min, <=max
bounds, @lifetheater57? I don't think there's python tooling that can do that, and even if there was the number of test cases would explode -- and our tests already take half an hour per commit.
I think the best we can do is set the max
bound to the current latest torch
(same with our other dependencies). If we ever miss updating our max
for one of their releases we'll just have to assume it worked if the later one did. And if it doesn't we can go find the most recent version that did and pin to that.
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.
I ran into a real-world example of lacking an upper bound causing problems last week: zalandoresearch/pytorch-ts#105
This project is declares gluonts>=0.9.0
, sometime in the last few months gluonts moved their API all around without even writing a good changelog, so now torchts==0.6.0
is broken forevermore. pip install torchts==0.6.0
will always produce a broken install, so there's no way for people to go back in time and try an old version.
When I crashed into this I scrambled, and looked at the release dates of both projects to figure out that gluonts==0.9.3
was the current version at the time of it's release, and I was able to get it working with:
pip install 'torchts' 'gluonts<0.9.4'
So we will be a lot friendlier to the future if we put <max
bounds on every one of our dependencies.
SCT had the same problem, by the way, but they solved it by ignoring most of python's packaging conventions; instead, making a release means generating and committing requirements-freeze.txt
to the release
branch, and then the (homegrown, unconventional, non-pip
) install script will install all of them. I don't think that's good either because it overspecifies the dependencies and requires all this fragile shell scripting to get it right, and then it's impossible for anyone to build on SCT because SCT controls the entire environment.
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.
No, I would only test the min and max versions of the libraries we "manually" depends on if it is possible to automate that. For example, I would do it for PyTorch, but not for a library that is automatically installed by PyTorch.
Excellent, I agree.
What are the major downsides of going the "rolling releases" way?
I'm not totally sure. So far I've thought up two: it would leave us open to your original concern of surprise breakage, and it would mean that past versions will be broken forever, instead of just what's on the master
branch.
I think for the sake of this PR, I am happy to leave the upper bound. We can keep thinking about this and decide the best practice for all our dependencies later.
I think this PR is probably ready to merge.
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.
What are the major downsides of going the "rolling releases" way?
I'm not totally sure. So far I've thought up two: it would leave us open to your original concern of surprise breakage
In practice the "surprise breakage" of rolling releases means no one can install a working ivadomed, so we need to be able to issue hotfixes rapidly.
On the other hand, pinning dependencies doesn't avoid breakage, it just makes it someone else's problem: anyone who is trying to combine ivadomed with, say, a pytorch script they wrote that needs the very latest torch will be stuck. Though they can work around it with a virtualenv.
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.
tl;dr:
We can?
rolling releases: entirely remove dependency specifiers
- document: the only ivadomed we expect to work is "right now"
- automate: use a cronjob to test
master
against all the latest dependencies every daypinned releases: put
<max
bounds on all our dependencies
- automate: rely on Dependabot to remind us bump + test when new releases come out
I pondered a bit upon this and I don't feel we're there yet to go with rolling releases, especially thinking about the integration of Multi-Contrast Multi-Session. I'm quite content with pinned releases as of now.
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.
Thanks for all your thoughts. I'm going to start a new issue to keep talking about all this.
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.
-> #1191
Checklist
GitHub
PR contents
Description
Linked issues