Fixes #352, run test about onnxmltools #465
Conversation
.azure-pipelines/linux-conda-CI.yml
Outdated
| @@ -19,10 +19,11 @@ jobs: | |||
| numpy.version: '>=1.18.1' | |||
| onnx.version: '==1.7.0' | |||
| onnx.target_opset: '' | |||
| onnxrt.version: '-i https://test.pypi.org/simple/ ort-nightly' | |||
| onnxrt.version: 'onnxruntime==1.3.0' # -i https://test.pypi.org/simple/ ort-nightly' | |||
There was a problem hiding this comment.
Could you please clean-up commented out code?
There was a problem hiding this comment.
I let them on purpose to have an easy to way to test against nightly build as I never remember this syntax.
There was a problem hiding this comment.
Not sure if that's a good reason to have commented code. You can always look at the history and see the previous version.
There was a problem hiding this comment.
I'll remove it but my point is people look for something in history if they think can find it there. I doubt this would be the case for this.
.azure-pipelines/win32-conda-CI.yml
Outdated
| @@ -20,9 +20,11 @@ jobs: | |||
| onnx.target_opset: '' | |||
| numpy.version: 'numpy>=1.18.1' | |||
| scipy.version: 'scipy' | |||
| onnxrt.version: '-i https://test.pypi.org/simple/ ort-nightly' | |||
| onnxrt.version: 'onnxruntime==1.3.0' # -i https://test.pypi.org/simple/ ort-nightly' | |||
There was a problem hiding this comment.
Same comment as above.
| - script: | | ||
| call activate skl2onnxEnvironment | ||
| if "$(onnx.target_opset)" neq "" set TEST_TARGET_OPSET=$(onnx.target_opset) | ||
| python -m pip install git+https://github.com/xadupre/onnxmltools.git@i396xgb |
There was a problem hiding this comment.
Why is there a reference to your forked branch?
There was a problem hiding this comment.
Because this one has not yet been merged and is needed.
There was a problem hiding this comment.
Shoudn't we first merge the necessary changes then? Not sure if it is a good idea to reference a forked branch.
There was a problem hiding this comment.
You're right. That's my plan. I'm waiting the PR onnx/onnxmltools#397 to be merged.
|
I am not sure if it is good to make a cross dependency here. onnxmltools is a wrapper for skl2onnx, and it depends on skl2onnx. So if there is a functional test or integration test, the test should be made in onnxmltools, and if it is a unit test, which is not supposed to depend on onnxmltools. |
|
This test ensures that xgboost and lightgbm can be used in a scikit-learn pipeline and still be converted into a single onnx file. I think we need to detect if this mechanism is broken. It depends on the ability to register new converters and it was introduced to support onnxmltools converters for lightgbm and xgboost. What do you suggest instead? |
Unless there are some internal skl2onnx APIs used in these test cases, it would be great to have these tests in onnxmltools to demonstrate how multiple converters can work together. |
|
I can move the test in the nightly build if you prefer but I think we should continue testing the conversion of lightgbm and xgboost in pipelines. We should have the same test in onnxmltools. Whenever these tests break in a PR, the author make sure this continue to pass, even if it means keeping depracated code for a couple of releases. |
No description provided.