-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Dependencies: torch 1.9+ required #936
Conversation
Thanks! It looks like |
Looks like 1.9.0 also has issues. Do these make sense to anyone? I'll keep bumping the min version until it works, but let me know if these make sense. |
Possibly related to #937 |
Torch 1.9 + dims fix looks fine, loss tests passed for minimal version |
We can just merge #937 |
Do you want to make all tests required now? This will force even older PRs to be rerun before merging. |
Can you provide a bit more details? I was actually thinking about reducing the number of platforms in PRs, or maybe conditioning it on some label, but not sure how this can be implemented 🙂 |
Basically, make main a protected branch that can only be merged to when a certain set of CI tests pass.
You can either include or exclude certain combinations until you get the desired result. But I wouldn't worry too much about the additional CI time, it's free. |
#927 uses
torch.linalg.vector_norm
, which requires torch 1.9+. This wasn't caught in CI because the tests weren't run after #918 was merged.P.S. We should require all tests to pass before a PR can be merged. Let me know if you need help updating the branch protection rules.