Repository navigation
Drop version checks for long-unsupported dependency releases - #3093
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
vLLM parser imports examples/vllm_serve/vllm_serve_fakequant.py, examples/vllm_serve/vllm_serve_sparse_attn.py |
Both examples import FlexibleArgumentParser from vllm.utils.argparse_utils without selecting the import by vLLM version. |
ONNX Runtime gates modelopt/onnx/quantization/ort_utils.py, tests/unit/onnx/autocast/test_nodeclassifier.py |
The DDS path no longer removes "trt" based on the ONNX Runtime version. Four node-classifier tests no longer skip when the version is below 1.21.0. |
Transformer Engine GEMM selection and support modelopt/torch/quantization/plugins/transformer_engine.py, CHANGELOG.rst, docs/source/getting_started/_installation_for_Linux.rst |
LayerNormLinear forward and backward quantization capture and patch te_layernorm_linear.general_gemm directly. The changelog and Linux requirements state that Transformer Engine 2.0 or later is required. |
Megatron rotary calls modelopt/torch/speculative/plugins/megatron_eagle.py |
The Megatron-Core version helper was removed. RoPE and YaRN rotary-sequence-length calls now always pass packed_seq_params=None. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Refactor
Suggested reviewers: cjluo-nv, chenhanyu, kinjalpatel27
Merge Risk: 
c8867
-
The dependency-gate removals are consistent with the supported versions, with no identified merge-blocking behavior change.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: removing compatibility checks for unsupported dependency versions. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The PR adds no torch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardcodedtrust_remote_code=True,eval()/exec(), or# noseccomments. It also changes no `pypr…Full details: Security Anti-Patterns
-
Explanation
PASS. The PR adds no
torch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardcodedtrust_remote_code=True,eval()/exec(), or# noseccomments. It also changes nopyproject.tomlor requirements file. The Python diff only removes dependency-version compatibility branches and updates related documentation/tests.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 4 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
Nudge: CI versions do not establish that the optional-backend compatibility branches are unreachable in supported local installations.
Needs action:
- Explain why deleting TE/Megatron guards is safe for supported local installs rather than retaining guards or enforcing minimums;
pyproject.tomlconstrains neither dependency. Cite existing exclusions, or make the support change explicit. - Verify the TensorRT-LLM import boundary in
modelopt/deploy/llm/generate.py: cite the API excluding <1.1.0rc2 or test that boundary before removing the capability gate. Reconcile the installation guide’s advertised >=1.0 support.
No action needed:
- The ONNX test edits are justified by the supported-platform ORT dependency floors; all assertions remain intact.
There was a problem hiding this comment.
Claude review — Drop version checks for long-unsupported dependency releases
Scope: full review (trigger comment carried no scoping instructions). All 7 changed files reviewed — 6 source (modelopt/ ×4, examples/ ×2) and 1 test. Nothing deliberately skipped.
Findings
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| IMPORTANT | 0 |
| SUGGESTION | 2 |
Both SUGGESTIONs are dead-abstraction cleanups left behind by the version branches this PR removes, and neither blocks merge:
transformer_engine.py:316-322—_get_original_gemm/_gemm_replace_argsare now constant indirection; inlining them at the four call sites removes the abstraction whose only purpose was version dispatch.generate.py:169—gather_context_logitsis a constant-Trueproperty with no in-repo reader. Either delete it or document that it is retained for backward compatibility.
What I verified
Each of the five removals is behavior-preserving within the supported stack, and the dead-code claims in the description hold up:
- onnxruntime
>=1.21— everyonnxruntime*pin in the[onnx]extra is>=1.22(pyproject.toml:67-76: 1.29, ~1.24.2, 1.22.0, ~1.26.0), so the assert could never fire. Removing it also drops a latent bug: the secondcalibration_eps.remove("trt")sat inside the sameif "trt" in calibration_epsguard after the TRT-10.6 branch could already have removed"trt", so a host with both TRT <10.6 and ORT <1.21 would have raisedValueError: list.remove(x): x not in list. The real guard,_check_for_tensorrt(min_version="10.6"), is untouched. - TensorRT-LLM
1.1.0rc2— confirmed the module already hard-fails below that floor: thetry: from tensorrt_llm.llmapi import CudaGraphConfig ...block atgenerate.py:27-34ends inprint("Please upgrade tensorrt-llm to 1.1.0rc2 or later"); raise. So_support_context_logits_and_stop_wordswasTruein every importable environment — dropping thestop_words = Nonereset in_generateand the assert ingenerate_context_logitschanges nothing. The_enable_kv_cache_reuseassert, which guards the genuinely incorrect case, is preserved. - Transformer Engine
<2.0— the two_patched_general_gemmclosures are forward-vs-backward (dgrad/wgrad call-order counter), not version variants, so collapsing togeneral_gemmleaves the GEMM argument order and the STE backward path intact. - megatron-core
>0.9.0— both survivingget_rotary_seq_lencall sites (megatron_eagle.py:939rope,:948yarn) now passpacked_seq_params=Noneas a keyword, which matches mcore's signature onRotaryEmbeddingand the Yarn subclass. Noextra_kwargsreference survives. Also checked that droppingimport megatron.coreis safe: every remainingmegatron.coreoccurrence in the file is afrom ... import, a registry string, or a docstring — no bare attribute access that wouldNameError. - vLLM
<=0.11—vllm.utils.argparse_utilsexists for everything above 0.11, and the stack is far above it (README installs 0.30; the sparse-attention runtime hard-errors below 0.15 atvllm_runtime.py:173). The retainedmake_arg_parsertry/except for pre-0.29 is consistent with that, not contradicted by it. No leftovervllm/versionreferences in either example after the import removals. - No orphaned references —
mcore_version_higher_than,_support_context_logits_and_stop_words, and_TE_VERSIONhave zero remaining uses repo-wide;ort,Version,te, and thetensorrt_llm-scoped imports are all still needed where they were kept, so no unused imports were introduced. - Test change — the four unskipped
test_nodeclassifier.pytests are CPU-only and now always run, matching the>=1.22floor; the description reportstests/unit/onnxat 498 passed.
One thing worth a sanity check before merge, not a finding: mcore_version_higher_than was a module-level name without a leading underscore and the module declares no __all__, so it was nominally importable by downstream code (e.g. Megatron-LM's examples/post_training/modelopt scripts). There are no in-repo users, and the body was a one-line version compare that a downstream caller can trivially restate, so I do not think this needs a deprecation shim — just confirming you have ruled out an external importer.
Risk assessment
Low. This is a subtractive cleanup with no install-metadata changes, no modelopt_state schema or mode-registration impact, and no change to quantization math, export layout, or collective ordering. Every removed branch is unreachable given the declared pins or the module's own import-time guard, and the one behavior change that is reachable — the ORT/TRT double-remove path — is a fix rather than a regression. Residual exposure is limited to users on a TE 1.x install, who would now hit a cryptic AttributeError on general_gemm instead of a working path; given TE 2.0 shipped in early 2025 and nemo:26.08 carries 2.17, that is an acceptable trade and consistent with the rest of the PR.
Approving.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@cjluo-nv re the bot review: ModelOpt's Megatron-Bridge / Megatron-LM features already require |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3093 +/- ##
==========================================
+ Coverage 78.09% 79.78% +1.69%
==========================================
Files 662 662
Lines 76116 76094 -22
==========================================
+ Hits 59439 60710 +1271
+ Misses 16677 15384 -1293
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:
|
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
Nudge: the TensorRT-LLM concern is resolved, but the Megatron container requirement does not establish a support floor for standalone TE quantization.
Needs action:
- 💬 Author cites
nemo:26.08for Megatron — clarify standalonete.pytorch.LayerNormLinearsupport in the PR body; cite an existing TE>=2.0 requirement, retain the fallback, or document the support change in installation docs andCHANGELOG.rst.
No action needed:
- ✔️ Resolved since the last review: TensorRT-LLM gate restoration, TE helper inlining, and the Megatron support-floor rationale.
- The ONNX skip removals are justified by the
[onnx]dependency pins; all assertions remain intact.
a982a32 to
359bcc7
Compare
|
@cjluo-nv re standalone TE quantization: made the support change explicit in 359bcc7. The install guide now lists Transformer Engine >=2.0 (optional), and |
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
LGTM: prior compatibility concerns are resolved; human sign-off remains required for the justified ONNX test edits.
Needs action:
- Review and sign off on
tests/unit/onnx/autocast/test_nodeclassifier.py: the removed skips match supported ORT dependency floors, and every assertion remains intact.
No action needed:
- ✔️ 4 previous concerns addressed, including standalone TE support documentation and TensorRT-LLM gate restoration.
Remove branches for onnxruntime<1.21, Transformer Engine<2.0, megatron-core<=0.9, TensorRT-LLM<1.1.0rc2 and vLLM<=0.11. Every supported install or CI container is well past these versions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
TensorRT-LLM 1.0 still imports this module (CudaGraphConfig exists there), so the <1.1.0rc2 context-logits/stop-words gate is not dead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
359bcc7 to
c8867eb
Compare
ChenhanYu
left a comment
ChenhanYu
left a comment
There was a problem hiding this comment.
Pensieve nudge review using chenhany's profile (revision 8).
Nudge: prior compatibility concerns are resolved, but vLLM launcher validation and current-head test results remain unverified.
Needs action:
- Confirm current-head ONNX/TE GPU results and record vLLM launcher validation in
Testing: run the existing fakequant example tests and a sparse-launcher import smoke check. Execution and CI logs are unavailable here. - Confirm human sign-off on
tests/unit/onnx/autocast/test_nodeclassifier.py, continuing review5694264690; removing skips matches supported ORT floors and preserves every assertion.
No action needed:
- ✔️ Resolved since earlier reviews: standalone TE support documentation, TensorRT-LLM gate restoration, TE helper inlining, and the Megatron support-floor rationale.
- The surviving TE2 and Megatron paths preserve the base behavior; existing TE tests exercise quantization, backward, and restore. ONNX test coverage expands rather than shrinks.
- No new defects or licensing changes identified. Duplicate checking covers the supplied bounded history only; reported test passes were not independently executed.
Review context was incomplete.
Review run: 5e883ebf2b194e1b8635d27791968e19
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
LGTM: prior compatibility concerns are resolved; only human sign-off on the justified ONNX skip removals remains.
Needs action:
- Review and sign off on
tests/unit/onnx/autocast/test_nodeclassifier.py: all four removed skips match supported ORT dependency floors, and every assertion is preserved.
No action needed:
- ✔️ 4 previous concerns addressed, including standalone TE support documentation and TensorRT-LLM gate restoration.
…on each rank's own experts (#3122) > [!NOTE] > **[1/2] of #3074's split.** A reviewer asked on #3074 for its expert-parallel correctness fix to land on its own, with a Bug Fixes changelog entry, apart from #3074's two backward-breaking changes (calibration on `grouped_mm` by default, and the new `calibrate()` error). This PR is that fix. #3074, now [2/2], sits on it and keeps the rest. > > **Base:** `main` (`2b900c8fa41`). ### What does this PR do? Type of change: Bug fix Under the expert parallelism of transformers 5.16 and later (the `grouped_gemm` plan style), a fused experts module keeps each expert weight as a DTensor sharded over the experts (`Shard(0)`) and sets `num_experts` to the rank's count. Its forward runs on local tensors, but ModelOpt also reads the expert weights outside the forward, by local expert index: - max calibration's weight pass and `fold_weight`, through `get_expert_weight`; - the weight-input hooks of local Hessian and imatrix (`on_weight` at registration, and the eager per-expert capture); - GPTQ's per-expert helpers; - the native FP8 wrapper's dequantization, which also indexes `*_scale_inv`. An index into the DTensor names a global expert, and each such read all-gathers every expert. (Before 5.16, transformers kept each rank's slice as a plain tensor, which these reads indexed correctly.) On two ranks with a 4-expert Qwen3-MoE experts module under weight-only INT8, main calibrates rank 1's two weight quantizers on rank 0's experts 0 and 1 instead of its own experts 2 and 3. `fold_weight` folds into the gathered copies, so no rank's own experts get folded, rank 0's included. `_QuantFusedExperts._local_experts` hands those readers the rank's own shard, in quantizer order. A DTensor holding every expert (FSDP2) is indexed as before. Two layouts are refused instead of read wrong: - a DTensor whose local shard does not hold the module's experts (one replicated over the ranks, say), which would otherwise be indexed as global experts; - native FP8 expert weights sharded over the experts, in `unpack_weight`, which would otherwise unpack into a buffer for every expert from this rank's experts. Both projections are checked before either is unpacked. ### Usage No API change. ### Testing - New tests in `tests/unit/torch/quantization/plugins/test_fused_experts.py` run on CPU over two gloo ranks, with experts sharded as transformers shards them. All six cases fail on main and pass here: - weight calibration and `fold_weight` use each rank's own experts: plain, and with the dense part FSDP2-sharded, on named and unnamed meshes; - a replicated expert DTensor, and native FP8 experts sharded over the experts, are refused; - registering the weight-input hooks hands `on_weight` each rank's first local expert, and runs no collective; - GPTQ builds its expert helpers from each rank's own experts. - `tests/unit/torch/quantization` on CPU (torch 2.14, transformers 5.18): the six new cases pass, and every other result matches main's. - pre-commit, mypy included, passes against main. - Review suggestion (Claude), 2026-10-29: `unpack_weight` checks both projections before unpacking either, so a refusal leaves the module unchanged; the refusal test shards `down_proj` alone and checks that `gate_up_proj` is still FP8, which fails with the per-projection check. The changelog entry and `_local_experts`' docstring name transformers 5.16, where expert-parallel weights became DTensors. pre-commit passes against main. - Rebased on 2026-10-30 onto main `99c3834f62c` (after #3093, #3108, #3101, #2737, #3099 and #2750 merged), with its own diff unchanged and no conflicts. pre-commit, now with main's ruff 0.17 and mypy 2.4 hooks, passes. ### Before your PR is "*Ready for review*" Make sure you read and follow [Contributor guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md) and your commits are signed (`git commit -s -S`). Make sure you read and follow the [Security Best Practices](https://github.com/NVIDIA/Model-Optimizer/blob/main/SECURITY.md#security-coding-practices-for-contributors) (e.g. avoiding hardcoded `trust_remote_code=True`, `torch.load(..., weights_only=False)`, `pickle`, etc.). - Is this change backward compatible?: ✅ <!--- A weight layout the quantizers cannot index now raises instead of calibrating the wrong experts. --> - If you copied code from any other sources or added a new PIP dependency, did you follow guidance in `CONTRIBUTING.md`: N/A - Did you write any new necessary tests?: ✅ - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: ✅ (Bug Fixes) - Did you get Claude approval on this PR?: ❌ <!--- Requested. The same code was reviewed and approved as part of #3074. --> ### Additional Information Split out of #3074 at a reviewer's request. Jira: OMNIML-6073. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed weight calibration, GPTQ quantization, and weight folding for Hugging Face fused MoE models split across expert-parallel ranks. Each rank now uses its local experts’ weights. * Unsupported expert-sharding layouts now return a clear error when weights cannot be safely indexed or unpacked. * **Improvements** * Preserved native scaled FP8 expert weights through quantization and export, with exported weights checked against reconstructed BF16 references. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
What does this PR do?
Type of change: Cleanup (drops Transformer Engine 1.x support)
Removes version checks for dependency releases that no supported install or CI container can have anymore. No install metadata changes.
onnxruntime<1.21test skips intest_nodeclassifier.py; the>=1.21DDS-op check inort_utils.update_trt_ep_support[onnx]extra is >= 1.22<2.0branches in_QuantLayerNormLinearFunc, and the two helpers that only dispatched on itnemo:26.08has TE 2.17mcore_version_higher_than("0.9.0")aroundpacked_seq_paramsin EAGLEnemo:26.08has 0.19<=0.11FlexibleArgumentParserimport branches inexamples/vllm_serveThe vLLM >= 0.15 check in the sparse-attention plugin and the
lm_eval>=0.4.12checks inexamples/llm_evalstay: they are real errors that match their requirement floors. So does the TensorRT-LLM<1.1.0rc2context-logits / stop-words gate inmodelopt/deploy/llm/generate.py: TensorRT-LLM 1.0 (the install guide's floor) still imports that module.TE and megatron-core are not in
pyproject.toml; ModelOpt's Megatron-Bridge / Megatron-LM features requirenemo:26.08(CHANGELOG), which ships TE 2.17 and megatron-core 0.19. For standalone TE quantization, the install guide now lists Transformer Engine >=2.0 andCHANGELOG.rstnotes that TE 1.x is no longer supported (its last release, 1.13, is from Dec 2024).Usage
N/A
Testing
tests/unit/onnx(incl. the previously gatedtest_nodeclassifier.pytests): 498 passed.nvcr.io/nvidia/nemo:26.08(TE 2.17.1, megatron-core 0.19):tests/gpu_megatron/torch/quantization/plugins/test_transformer_engine.pypasses (21 passed, 4 skipped),RotaryEmbedding.get_rotary_seq_lenacceptspacked_seq_params, and the EAGLE Megatron plugin imports.Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/AAdditional Information
Companion to #3091 (torch 2.10 floor) and #3092 (transformers / diffusers floors).
🤖 Generated with Claude Code
Summary by CodeRabbit