Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the version pinning for onnxruntime on Windows, which is a good step to simplify dependencies if testing confirms it's stable. I've added one suggestion to refactor the remaining onnxruntime dependency specifiers for better readability. Also, once this change is validated and ready for merging, please remember to update the CHANGELOG.md to reflect the removal of this pinning.
| "onnxruntime>=1.17.0 ; python_version > '3.9'", | ||
| "onnxruntime>=1.17.0, <1.20.0 ; python_version <= '3.9'", |
There was a problem hiding this comment.
For better readability and to reduce redundancy, you can simplify the onnxruntime dependency specifications. You can declare the base requirement for all versions and then add the conditional upper bound for older Python versions on a separate line. This makes the dependency logic easier to follow.
| "onnxruntime>=1.17.0 ; python_version > '3.9'", | |
| "onnxruntime>=1.17.0, <1.20.0 ; python_version <= '3.9'", | |
| "onnxruntime>=1.17.0", | |
| "onnxruntime<1.20.0 ; python_version <= '3.9'", |
- add pre-release-checks, gated by release tags - run package checks on the actual artifacts (i.e., the packages), and not just from the source of the repo. - add stub for actual release - simplify "uv add magika" linux vs. windows, using shell: bash - make uv version easily updatable
In GH actions, the "path" filter is not used for tags push. This means that any tag push would trigger this gh action, even if the push has nothing to do with it. Fixed by triggering only for branch pushes to main (note: the branch filter is used for tags as well), and for any PR that relates to this.
…ot needed if we don't do that, the build-wheels and other jobs will not run as the pre-release-checks is a dependency.
84bbc93 to
6989f2d
Compare
1 participant
At some point we added pinning for windows for onnxruntime (see #1099).
Now I'm trying to check whether we could avoid that, by using what's suggested at astral-sh/uv#9425 (comment).
Update: upon further inspection, the reason we had the onnxruntime pin for windows was not to "guide" uv, but to make sure that, on windows, we pick a version of onnxruntime that is old enough to not require the new VC++ requirement. So, the workaround mentioned in the uv's issue does not seem to be related to this problem. We'll keep the pinning for now.