Repository navigation
[Experimental] Update ONNX extras - #40821
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR updates the onnx extra and ml_base_core dependency bounds in setup.py, re-enables the ONNX postcommit test tasks in tox.ini and build.gradle (testing lower-bound 119 and latest), and disables multi-process model sharing on OnnxModelHandlerNumPy since onnxruntime.InferenceSession manages its own native thread pools and cannot be safely proxied across processes.
🔍 General Feedback
- The changes to
OnnxModelHandlerNumPyand the updated dependency/tox configurations are well-structured and clearly documented. - Note that the branch currently has a merge conflict with recent
masterchanges (from PR #40752) in.github/trigger_files/beam_PostCommit_Python_Dependency.jsonand adjacent lines insdks/python/test-suites/tox/py311/build.gradle/sdks/python/tox.ini, so a rebase onmasterwill be needed. - One minor cleanup opportunity was noted in
tox.iniregarding an obsoleteonnx<1.19.0pin intestenv:py311-ml.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @tvalentyn for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40821 +/- ##
===========================================
+ Coverage 0 79.90% +79.90%
===========================================
Files 0 493 +493
Lines 0 83479 +83479
===========================================
+ Hits 0 66706 +66706
- Misses 0 16773 +16773
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # Converters for producing ONNX models from scikit-learn, | ||
| # TensorFlow and PyTorch models. | ||
| 'skl2onnx>=1.17.0,<2', | ||
| 'tf2onnx>=1.17.0,<1.18', |
There was a problem hiding this comment.
do you think the upper bounds need to be so tight for this and the below? I'm trusting your judgement.
There was a problem hiding this comment.
They do, unfortunately. There's some juggling between compatible versions of tensorflow and versions of torch that can exist in the same environment, which is not ideal but does keep the extra here working out-out-the-box.
|
Should we cherrypick this into release-2.78 branch? |
|
@Amar3tto Yeah I can open a cherrypick, this isn't a release blocker necessarily but ONNX was not going to work on 2.78.0 otherwise. |
Modernize the ONNX extras group and re-enable testing, which has been disabled since 2024. Prior to updating, the onnx extras group was not able to install on any currently supported Python version.