-
Notifications
You must be signed in to change notification settings - Fork 12
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
Run tests also on python 3.10 #50
Conversation
Kudos, SonarCloud Quality Gate passed! |
This is blocked on onnxruntime releasing a pypi package for 3.10, see microsoft/onnxruntime#9782. |
Are we still considering 3.10? |
Patrick's comment still applies I think. We should support 3.10 when we can (which just means running the tests, there's nothing breaking in 3.10) |
Kudos, SonarCloud Quality Gate passed! |
FYI, I rebased on main. |
onnxruntime 1.12 is out and it has a 3.10 pypi release! I rebased this branch on main, hope the tests pass now 🤞 |
Ok, the whole thing seems to pass now, but still have to remove three commits that are no longer necessary, will rebase now... |
I have no idea why linting fails now, because I didn't change any linted code. I think it should also be an issue on the main branch right now, but haven't seen it there. |
I have also added a sentence at the Tutorials readme and the linting failed :-(. @egpbos , any ideas? |
Ok, pylint uses astroid and astroid crashes (for some reason) with a recursion depth error. This error was fixed in astroid 2.12.2 (the most recent version), but pylint has set an upper limit on its astroid requirement with I manually installed the more recent astroid version on my laptop and I see no linting errors (except actually a new one that is only present for Python 3.10, but unrelated to this PR... let's fix that when we switch fully to 3.10 for CI default). So I think we can safely merge this now. We could in theory add a manual install step to install the latest astroid manually on CI, but I think it would be better to just wait for the upstream fix and be careful in the meantime (i.e.: check linting on laptop). So, in short: ready to merge :) |
Ah, yeah, we just have to wait for the next pylint version for this issue to resolve itself, the dependency was already updated in pylint: pylint-dev/pylint#7153 |
@elboyran once you give an approving review, the PR will be merged. |
@egpbos how to update the badge of supported versions? |
That should go automatically after a new release. The badge shows versions on PyPI. |
Closes #49