Add onnxsim as an alternative ONNX simplification backend#2018
Add onnxsim as an alternative ONNX simplification backend#2018take-cheeze wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe quantization workflow adds selectable ONNX simplification backends, exposing ChangesONNX simplification backend selection
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant quantize
participant _preprocess_onnx
participant onnxslim_or_onnxsim
User->>CLI: select --simplify_backend
CLI->>quantize: pass simplify_backend
quantize->>_preprocess_onnx: request simplification
_preprocess_onnx->>onnxslim_or_onnxsim: invoke selected backend
onnxslim_or_onnxsim-->>_preprocess_onnx: return simplified model
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
345acfa to
d98e6c9
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/onnx/quantization/test_quantize_api.py (1)
92-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise backend selection rather than only updating mock arity.
Capture and assert the forwarded default, then add focused tests for
onnxsim, unsupported backends, and missing optional dependencies.Minimal assertion
simplify_backend, quantize_mode, opset, ): + captured["simplify_backend"] = simplify_backend return onnx_path, object(), [], True, False, False, {}, {} ... + assert captured["simplify_backend"] == "onnxslim"As per path instructions, tests must add coverage for new features and exercise the behavior they validate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/onnx/quantization/test_quantize_api.py` around lines 92 - 95, Expand the quantization API tests around the backend-selection parameter in the test function receiving simplify_backend, quantize_mode, and opset: capture and assert its forwarded default, then add focused coverage for the onnxsim backend, unsupported backend handling, and missing optional dependencies. Ensure the tests exercise the actual selection behavior rather than only adjusting mock argument arity.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/onnx/quantization/quantize.py`:
- Line 401: Reorder the new simplify_backend parameter in the quantization
function signature so it appears after autotune_trtexec_args, preserving the
existing positional binding of later parameters such as calibrate_per_node. Keep
its default value unchanged and update any related signature handling
accordingly.
---
Nitpick comments:
In `@tests/unit/onnx/quantization/test_quantize_api.py`:
- Around line 92-95: Expand the quantization API tests around the
backend-selection parameter in the test function receiving simplify_backend,
quantize_mode, and opset: capture and assert its forwarded default, then add
focused coverage for the onnxsim backend, unsupported backend handling, and
missing optional dependencies. Ensure the tests exercise the actual selection
behavior rather than only adjusting mock argument arity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22ab02b7-001d-4812-ae75-19e815c5d0fc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.rstmodelopt/onnx/quantization/__main__.pymodelopt/onnx/quantization/quantize.pypyproject.tomltests/unit/onnx/quantization/test_quantize_api.py
Keep onnxslim as the default simplifier and offer onnxsim as an equivalent, user-selectable option for modelopt.onnx.quantization.quantize(..., simplify=True) and the --simplify CLI flag. - quantize.py / __main__.py: add a simplify_backend argument (--simplify_backend) that selects between "onnxslim" (default, unchanged behavior) and "onnxsim". An unknown backend name or a missing onnxsim install fails loudly; genuine simplification failures still fall back gracefully to the original model. - pyproject.toml / uv.lock: add onnxsim>=0.7.0 to the onnx extra alongside onnxslim. onnxsim 0.7.0 ships wheels for Python 3.12+ and aarch64. - test_quantize_api.py: update the _preprocess_onnx mock signature for the new argument. - CHANGELOG.rst: document the new backend option under 0.47. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PYET7LC6SwmWpkCSN18T1Z Signed-off-by: Claude <noreply@anthropic.com>
d98e6c9 to
8c7f03c
Compare
What does this PR do?
Type of change: New feature
Adds
onnxsimas an alternative ONNX simplification backend formodelopt.onnx.quantization.quantize(..., simplify=True)and the--simplifyCLI flag, while keeping
onnxslimas the default. Previously the simplifierwas hard-wired to
onnxslim; this makes the two interchangeable via a newsimplify_backendargument (--simplify_backendon the CLI) that accepts"onnxslim"(default) or"onnxsim".Details:
quantize.py:quantize()and_preprocess_onnx()gain asimplify_backend: str = "onnxslim"argument. The simplify block dispatcheson it —
onnxslim.slim(..., skip_fusion_patterns=["FusionGemm"])foronnxslim,onnxsim.simplify(...)foronnxsim. An unknown backend name ora missing
onnxsiminstall fails loudly (raised before the guarded block),whereas genuine simplification failures still fall back gracefully to the
original model, exactly as before.
onnxsimis imported lazily so it is onlyrequired when actually selected.
__main__.py: adds--simplify_backendwithchoices=["onnxslim", "onnxsim"],default
onnxslim, wired into thequantize(...)call.pyproject.toml/uv.lock: addsonnxsim>=0.7.0to theonnxextraalongside the existing
onnxslim>=0.1.76.onnxsim0.7.0 ships wheels forPython 3.12+ and aarch64, so no platform markers are needed.
Usage
Testing
_preprocess_onnxmock intests/unit/onnx/quantization/test_quantize_api.pyfor the new argument;the existing unit suite for the quantize API continues to pass.
tests/gpu/onnx/test_simplify.pyexercises thedefault (
onnxslim) simplify path end-to-end.pyproject.tomlanduv.lockparse, the lock resolvesonnxsim0.7.0 (depsonnx,rich) withonnxslimuntouched, and thechanged Python files pass
ruff format --check.onnxsimpath(needs a GPU and
onnxsiminstalled).Before your PR is "Ready for review"
simplifydefaults to the existingonnxslimbackend; no existing call site or CLI invocation changes behavior.you follow guidance in
CONTRIBUTING.md: ✅ — addedonnxsimto the optionalonnxextra and relockeduv.lock; no code copied._preprocess_onnxmock only; a dedicatedsimplify_backend-selection unittest is not yet added (happy to add one covering the dispatch and the
invalid-backend
ValueErrorif reviewers want it).section.
/claude reviewonceopened.
Additional Information
No related issue. Default simplification remains
onnxslim;onnxsimis purelyadditive and opt-in.
Summary by CodeRabbit
--simplify_backendCLI option (andsimplify_backendparameter) to choose betweenonnxslimandonnxsimwhen model simplification is enabled.onnxsim>=0.7.0support for relevant Python versions and architectures.