DOC Add basic tests for ONNX support#7805
Conversation
Short example in the serialisation notebook and some tests to check that we can use cuml.accel with skl2onnx to convert models.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
github-actions
bot
added
conda
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/source/pickling_cuml_models.ipynb`:
- Around line 449-473: Add an explicit instruction and activation step for
cuml.accel so users actually train with cuML proxies before exporting to ONNX:
update the markdown around "Exporting to ONNX via cuml.accel" to state that the
notebook must enable cuml.accel (e.g., run the IPython magic %load_ext
cuml.accel or start the notebook with python -m cuml.accel) and add a small code
cell example showing activation prior to training (so that subsequent calls to
skl2onnx.convert_sklearn() operate on cuml.accel proxy estimators).
🧹 Nitpick comments (2)
python/cuml/cuml_accel_tests/test_onnx.py (2)
128-139: Consider guardingoptionsfor estimators within pipelines or meta-estimators.The
hasattr(model, "predict_proba")check on line 131 works for simple estimators, but for meta-estimators likeOneVsRestClassifier, thepredict_probaattribute may exist as a delegated method. Since those are alreadyxfail, this is fine in practice, but worth noting if the test scope expands.
172-196: Type-name dispatch is fragile for future extensibility.The
if name == "NearestNeighbors"/elif name == "KMeans"/elsebranching (lines 182–196) relies on class name strings. If cuml.accel proxies change the__name__of the type, or if new estimators with different output semantics are added, this will silently take the wrong branch.A more robust approach would be to check for capability (e.g.,
hasattr(estimator, "kneighbors"),hasattr(estimator, "predict")vshasattr(estimator, "transform")). That said, for a focused test file with a fixed set of estimators, this is acceptable.
betatim
added
non-breaking
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dependencies.yaml (1)
404-409: Consider adding version constraints foronnxruntimeandskl2onnx.Most core dependencies in this file carry version bounds (e.g.,
scikit-learn>=1.5,numpy>=1.23,<3.0).skl2onnxreleases are tightly coupled to scikit-learn versions, so a new incompatible release could silently break the ONNX tests or doc builds. Even a loose lower bound (e.g.,skl2onnx>=1.17) would help with reproducibility.
cuML's C++/CUDA SVC API is fundamentally designed to train binary estimators. In multi-class mode, the cuML SVC estimator trains a set of binary estimators (ovo/ovr). Scikit-Learn on the other end now relies on |
|
Just to make sure: The For me it is fine to keep the |
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
Thanks a lot for kicking this off!
|
@coderabbitai the "Docstring Coverage" check is not useful. dismissing that review |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/cuml/cuml_accel_tests/test_onnx.py (2)
1-37: Copyright header and xfail markers look good.The
strict=Trueon all xfail markers is the right choice — tests will alert you when upstream fixes resolve currently-expected failures.One consideration: the top-level imports of
hdbscanandumap(lines 4, 8) are only used for theunsupportedxfail entries. If either package is missing, the entire module (including all passing tests) becomes uncollectable. Since they're now declared as test dependencies this may be acceptable, but a defensive approach would be to guard them:hdbscan = pytest.importorskip("hdbscan") umap = pytest.importorskip("umap")This would skip only the relevant tests rather than failing the entire module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml_accel_tests/test_onnx.py` around lines 1 - 37, Top-level imports of hdbscan and umap make the whole test module uncollectable if those optional packages are missing; replace the direct imports with pytest.importorskip calls (i.e., set hdbscan = pytest.importorskip("hdbscan") and umap = pytest.importorskip("umap")) so only the tests relying on those packages are skipped; update the module-level names hdbscan and umap (the ones referenced when defining xfail markers like xfail_unsupported) accordingly.
159-171: Regressor test is reasonable.The
1e-2tolerance is fairly generous; for a tighter smoke test you could consider1e-4or1e-5for linear models likeLinearSVR, while keeping1e-2for tree ensembles. Not blocking, but worth revisiting if flaky tests appear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml_accel_tests/test_onnx.py` around lines 159 - 171, The test test_onnx_regressor currently uses a fixed tolerance of 1e-2 for all regressors; change it to use a tighter tolerance for linear models by computing a per-estimator threshold before the assert (e.g., set tol = 1e-4 or 1e-5 for linear estimators like LinearSVR and tol = 1e-2 for tree/ensemble estimators), detect the estimator via its class or name (estimator.__class__ or estimator.__class__.__name__ / isinstance checks), then replace the hard-coded 1e-2 in the assert with this tol variable so different model classes get appropriate tolerances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/pickling_cuml_models.ipynb`:
- Around line 449-463: Update the markdown cell under the "Exporting to ONNX"
section to use the HTML output filename for the cuml-accel link: replace the
relative link "cuml-accel/index.rst" with "cuml-accel/index.html" so the
notebook's literal hyperlink points to the built Sphinx dirhtml output; locate
the markdown cell that contains the sentence mentioning "Via `cuml.accel`" and
modify that hyperlink accordingly.
---
Nitpick comments:
In `@python/cuml/cuml_accel_tests/test_onnx.py`:
- Around line 1-37: Top-level imports of hdbscan and umap make the whole test
module uncollectable if those optional packages are missing; replace the direct
imports with pytest.importorskip calls (i.e., set hdbscan =
pytest.importorskip("hdbscan") and umap = pytest.importorskip("umap")) so only
the tests relying on those packages are skipped; update the module-level names
hdbscan and umap (the ones referenced when defining xfail markers like
xfail_unsupported) accordingly.
- Around line 159-171: The test test_onnx_regressor currently uses a fixed
tolerance of 1e-2 for all regressors; change it to use a tighter tolerance for
linear models by computing a per-estimator threshold before the assert (e.g.,
set tol = 1e-4 or 1e-5 for linear estimators like LinearSVR and tol = 1e-2 for
tree/ensemble estimators), detect the estimator via its class or name
(estimator.__class__ or estimator.__class__.__name__ / isinstance checks), then
replace the hard-coded 1e-2 in the assert with this tol variable so different
model classes get appropriate tolerances.
@coderabbitai don't comment suggesting such "defensive" imports, or in general with changes that are "defensive". The suggested changes tend to hide mistakes and legitimate errors. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
|
The docs build fails because we use scikit-learn and then enable the accelerator afterwards. I can't think of a good solution for this (we'd have to restart the kernel mid notebook, too much complexity I think). Possible fixes: remove the |
Right now the documentation is very clearly split between the "native cuML API" and "cuml.accel". I'd suggest that we maintain that clear separation. It might lead to some duplication, but that's ok. We can consider a more holistic reorganisation of our docs in a follow-up. |
|
This is ready for reviewing. The failing tests should be fixed by #7830 and are unrelated to this PR |
There was a problem hiding this comment.
I think this file was partially re-formatted. Would be good if we could avoid that in the future to keep the diff meaningful.
| xfail_unsupported = pytest.mark.xfail( | ||
| reason="not supported by skl2onnx", strict=True | ||
| ) | ||
| xfail_proxy_private_attr = pytest.mark.xfail( | ||
| reason="skl2onnx accesses private attributes not exposed by the proxy", | ||
| strict=True, | ||
| ) | ||
| xfail_skl2onnx_bug = pytest.mark.xfail( | ||
| reason="skl2onnx conversion error", strict=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
Would be great if we could track the unsupported estimators also in an issue.
|
/merge |
5 participants
This PR adds a short example in the serialisation notebook and some
cuml.acceltests to check that we can useskl2onnxto convert models.The
OneVs*meta estimators also get a low match score when I don't usecuml.accel, so I think this is a bug inskl2onnx- or something I don't understand about how to use it.SVRandSVCfail withcuml.accelbecause we miss a private attribute (_gamma). Not investigated how hard it would be to add it. https://github.com/scikit-learn/scikit-learn/blob/29cb1589e444a80b44fdd6911404a96a0e1e49ea/sklearn/svm/_base.py#L247-L259 makes me think not too hard.Fixes #7809