Conversation
Co-Authored-By: Codex <codex@openai.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change updates NVFP4 and FP8 ONNX precision handling. It adds FP4 graph opset normalization, preserves compatible BF16 paths, aligns casted output types, and adds regression tests for conversion errors and mixed-precision graphs. ChangesONNX precision conversion and NVFP4 export
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The NVFP4 precision conversion and graph-casting changes preserve dtype boundaries and are covered by CPU export regression tests. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2415 +/- ##
==========================================
+ Coverage 71.39% 78.94% +7.54%
==========================================
Files 590 590
Lines 64646 64675 +29
==========================================
+ Hits 46154 51056 +4902
+ Misses 18492 13619 -4873
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
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Nudge: the branch change itself is sound, but the new tests mock out both converters so nothing verifies the actual mixed-dtype fix, and the FP16 NVFP4 path now skips AutoCast's opset upgrade.
Needs action:
- Confirm the opset regression in
modelopt/torch/_deploy/utils/torch_onnx.py:convert_to_f16bumped NVFP4 graphs to opset 23 (get_min_opset_for_precisions), theconvert_float_to_float16branch does not, so default exports now ship opset-20 metadata over opset-23 FP4DequantizeLinear— fix it here or say why it's acceptable. - Add an end-to-end check that the exported FP16 NVFP4 graph has no mixed float-type elementwise inputs;
tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_exported_onnx_is_topologically_sortedshows a CPU-feasible NVFP4 export pattern. - Explain whether
weights_dtype="bf16"+ NVFP4 still hits the same TensorRT parse failure, since it keeps routing throughconvert_to_f16→GraphSanitizer.find_custom_nodes.
No action needed:
- The added
weights_dtype == "fp16"guard is a no-op for BF16 (the earlierValueErroralready covers FP8/other quantizers), so the BF16 path is genuinely preserved. - PR body notes the change is Codex-authored without Claude approval, so a human pass is warranted regardless.
Co-Authored-By: Codex <codex@openai.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
|
@cjluo-nv Addressed the three review items in
Validation: 128 affected CPU tests passed; the public Swin NVFP4 reproducer exported an opset-23 graph with zero mixed floating-type elementwise operations and completed a strongly typed TensorRT 11.1 engine build on Blackwell.
|
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Nudge: all three prior concerns are fixed with real code and real CPU coverage, but the PR quietly turns two previously-working NVFP4 conversions into hard errors and the agent-authored change still wants a human pass.
Needs action:
- Document the new hard failures in
CHANGELOG.rst:weights_dtype="bf16"with FP32 params + NVFP4, andweights_dtype="fp16"with BF16 params + NVFP4, previously ran throughconvert_to_f16and now raiseValueErrorinmodelopt/torch/_deploy/utils/torch_onnx.py. The PR body still marks this backward compatible. - Confirm the
weights_dtype="fp32"NVFP4 path: the newif uses_fp4:opset block runs there too andnvfp4_exporter.post_processnow stamps MatMul outputs as FP16, so that graph can still carry mixed FP16/FP32 elementwise inputs — say whether it is out of scope or add it totest_nvfp4_deploy_export_has_consistent_elementwise_types. - Have a human review the Codex-authored commits before merge.
No action needed:
- ✔️ Resolved since the last review: the opset-23 upgrade before precision conversion, the real CPU NVFP4 export test asserting no mixed-float elementwise inputs, and the BF16 no-op routing.
- Refactor of
test_nvfp4_exported_onnx_is_topologically_sortedinto_make_cpu_nvfp4_modelkeeps coverage intact.
Co-Authored-By: Codex <codex@openai.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
Co-Authored-By: Codex <codex@openai.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
|
Addressed the current-head review items in
The PR is now conflict-free. Current-head Blackwell/TensorRT acceptance has not been rerun, and human approval remains external.
|
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Nudge: the two prior code concerns are resolved with real CPU coverage, but hardware acceptance is stale on current head and one BF16 branch in the exporter is unexplained.
Needs action:
- Rerun or explicitly waive the Blackwell/TensorRT engine build for the current head; the PR body says it was not repeated after the final exporter change.
- Explain (ideally as a code comment) why the BF16 branch in
modelopt/onnx/export/nvfp4_exporter.py::_cast_input_dtypesre-stamps the node output value_info to BF16 instead of inserting a boundary Cast like the FP16 path — on a graph with any BF16 initializer this silently retypes outputs whose consumers are still FP32. - Have a human review the Codex-authored commits and the declared backward-incompatible
ValueErrorbehavior before merge.
No action needed:
- ✔️ Resolved since the last review: the
CHANGELOG.rstentry for the new FP32→BF16 / BF16→FP16 hard failures, and theweights_dtype="fp32"mixed-type path (Gemm bias cast + output boundary cast, covered by the new parameterizedelementwise/gemmCPU tests). - The output cast is skipped when a node output has no value_info (e.g. after a failed shape inference), which just restores pre-PR behavior rather than regressing.
What does this PR do?
Type of change: Bug fix
FP16 export of NVFP4-quantized ONNX graphs could invoke TensorRT parsing while elementwise operations still had mixed FP32 and FP16 inputs. TensorRT rejected the first mixed-type
Addbefore AutoCast could normalize the graph.This change:
DequantizeLinearnodes use their required opset-23 schema.MatMulandGemmcompute internally in FP16 or BF16, casts an optionalGemmbias to that compute type, and restores original FP32 graph boundaries with explicit output casts.MatMul,Gemm, and elementwise types, plus focused routing and conversion-guard coverage.Usage
Testing
Added regression-first coverage that exposed the opset-20 FP4 graph, the premature BF16 AutoCast/TensorRT path, mixed FP32/FP16
MatMuloutput boundaries, and aGemmwith a mismatched FP32 bias. Before the final fix, full ONNX checking failed withC has inconsistent type tensor(float).Ran the focused current-head CPU coverage with no GPU visible:
CUDA_VISIBLE_DEVICES="" python -m pytest -o addopts= -q \ tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_exported_onnx_is_topologically_sorted \ tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_deploy_export_has_consistent_elementwise_types \ tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_deploy_export_has_consistent_gemm_types \ tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_shared_activation_reuses_cast \ tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_packed_weights_match_eager \ tests/unit/torch/deploy/utils/test_torch_onnx_utils.py::test_nvfp4_export_rejects_unsupported_dtype_conversion \ tests/unit/torch/deploy/utils/test_torch_onnx_utils.py::test_nvfp4_export_selects_precision_converter \ tests/unit/onnx/quantization/test_qdq_utils.py::TestFP4QDQTo2DQ::test_fp4qdq_conversionResult: 20 passed in 1.03 seconds (19.93 seconds including container startup).
The real CPU regressions cover FP32 preservation, FP32-to-FP16 conversion, and native-BF16 no-op behavior across
MatMulplus explicit biasAddand biasedGemmtopologies. They output opset-23 graphs, retain dynamic FP4 activation quantization, pass full ONNX checking and strict shape inference, and verify the expected internal compute and graph-boundary types.Ran changed-file hooks after merging current
main. All applicable hooks passed, including Ruff, mypy, RST, license, and Bandit checks in 7.81 seconds.The Blackwell TensorRT engine build was not rerun after the final exporter change; current-head hardware acceptance remains pending.
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.).ValueErrorforweights_dtype="bf16"with FP32 source parameters and forweights_dtype="fp16"with BF16 source parameters, rather than attempting unsupported graph-wide conversions. Use FP32-to-FP16 export or native-BF16 export instead.CONTRIBUTING.md: N/AAdditional Information
weights_dtype="bf16"; graph-wide quantized FP32-to-BF16 conversion remains unsupported and fails early.weights_dtype="fp32"path keeps NVFP4 weighted operations in their supported low-precision compute type and restores FP32 at the surrounding graph boundary.Summary by CodeRabbit