-
Notifications
You must be signed in to change notification settings - Fork 101
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
Fixes for onnx==1.11.0 #824
Conversation
xadupre
commented
Feb 9, 2022
•
edited
Loading
edited
- remove many lines of codes introduced in the early stages of the development and not needed anymore
- introduce TARGET_OPSET_ML for testing purpose
- use TARGET_OPSET to enable, disable unittest instead of onnx.version
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Thanks @xadupre for doing this check, I can follow up on this if you want. |
It seems so. That's a surprising dependency. |
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>
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 the updates. Looks good. Left a few questions checking if my understanding is correct.
@@ -25,8 +23,7 @@ | |||
|
|||
class TestOnnxOperatorsCascade(unittest.TestCase): | |||
|
|||
@unittest.skipIf(StrictVersion(onnx.__version__) < StrictVersion("1.4.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.
Why does onnx "1.4.0" matches target opset "9"?
From
sklearn-onnx/tests/test_utils/__init__.py
Line 54 in d8803b8
def max_onnxruntime_opset(): |
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.
update: after looking at more changes, it seems the new opset version should be more accurate. Feel free to resolve if that's the case.
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 got it from here: https://github.com/onnx/onnx/blob/main/docs/Versioning.md.
@@ -21,8 +19,7 @@ class TestAlgebraComplex(unittest.TestCase): | |||
|
|||
@unittest.skipIf(Complex64TensorType is None, | |||
reason="not available") | |||
@unittest.skipIf(StrictVersion(onnx.__version__) < StrictVersion('1.8.0'), | |||
reason="not implemented") | |||
@unittest.skipIf(TARGET_OPSET < 13, reason="not implemented") |
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.
Similar to above should it be <14
?
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.
tests/test_algebra_converters.py
Outdated
@@ -18,8 +16,7 @@ | |||
|
|||
class TestAlgebraConverters(unittest.TestCase): | |||
|
|||
@unittest.skipIf(StrictVersion(onnx.__version__) < StrictVersion("1.4.0"), | |||
reason="not available") | |||
@unittest.skipIf(TARGET_OPSET < 10, reason="not available") |
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.
ditto
@@ -72,21 +61,13 @@ def test_ada_boost_classifier_samme_r_decision_function(self): | |||
) | |||
self.assertIsNotNone(model_onnx) | |||
dump_data_and_model( | |||
X_test, | |||
X_test[:5], |
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.
could you briefly explain this update?
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 forgot to remove it. I did it after adding verbose=True to understand the failure.
from onnxruntime import InferenceSession | ||
from test_utils import dump_data_and_model, TARGET_OPSET | ||
|
||
|
||
ort_version = '.'.join(ort_version.split('.')[:2]) |
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.
nit: this appears multiple time, maybe put into test_utils?
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.
Then the code can work with onnxruntime-training as well.
Signed-off-by: xavier dupré <xavier.dupre@gmail.com>