[6034518] Fix Conv->Relu->Concat Q/DQ insertion gap#1398
Conversation
|
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 Concat-group-aware atomic add/remove mutations to scheme sampling with a new config flag; integrates the mutation into insertion-point sampling probability; adds a Conv→(BN)→Add→Relu skip guard; promotes node-level insertions to tensor-level when uncovered consumers are Concat; and updates the autotuner skip-ops set. ChangesConcat-Group Quantization Mutations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/autotune/autotuner_base.py`:
- Around line 1150-1152: The computed concat_prob can exceed 1.0 (making the
branch deterministic) because concat_group_min_samples / num_schemes is
unbounded; change the calculation in autotuner_base.py where concat_prob is set
(using variables num_schemes and self.config.concat_group_min_samples) to clamp
the result to a valid probability range, e.g. ensure concat_prob =
min(max(self.config.concat_group_min_samples / num_schemes, 0.05), 1.0) so the
probability stays between 0.05 and 1.0 before the random.random() check.
- Around line 993-1016: The Concat mutation can be a no-op because action is
chosen unconditionally with random.choice(["add","remove"]); change the logic in
the mutation block in autotuner_base.py (the section using variables action,
absent_groups, full_groups, concat_groups, selected_points, selected_keys) so
you first compute available_actions based on whether absent_groups and/or
full_groups are non-empty, pick action from available_actions (or bail/return
early if none), and only then perform the add/remove branch; this ensures you
never pick an impossible action and avoids consuming a generation attempt with a
no-op.
🪄 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: 31b38904-2fc0-4237-b4cc-588e9eb16b79
📒 Files selected for processing (3)
modelopt/onnx/quantization/autotune/autotuner_base.pymodelopt/onnx/quantization/autotune/common.pymodelopt/onnx/quantization/autotune/insertion_points.py
dcd28ee to
563757d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/autotune/autotuner_base.py`:
- Around line 968-975: The concat_groups mapping is built from all_points and
may miss inputs filtered earlier, so treat a Concat as a valid group only if you
have a selectable point for every Concat input: for each group keyed by
p.node_index (concat_groups), fetch the Concat node arity (e.g., from the Concat
node's inputs/num_inputs on the node referenced by that index) and skip the
group unless len(concat_groups[node_index]) == concat_node_arity; apply the same
check in the similar block around the 982-991 logic that processes concat_groups
so you never select or sample on partial Concat groups (use the
functions/variables concat_groups, all_points, concat_local_indices,
selected_points and the Concat node's input count to implement the guard).
In `@modelopt/onnx/quantization/autotune/insertion_points.py`:
- Around line 372-395: The skip for "Conv -> [BN ->] Add" should only apply when
that Add actually feeds a Relu fusion target: update the block that handles
node.op == "Add" (the variables producer, conv_node, conv_act_input) to
additionally check the Add node's consumers and only return True if the Add's
output is consumed by a Relu (e.g., check node.outputs or the consumer nodes of
node and verify any consumer.op == "Relu" before returning True); otherwise fall
through so the quantization point is not incorrectly removed.
🪄 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: 0cb4da4a-b15d-4d02-9fd4-33368e34202b
📒 Files selected for processing (3)
modelopt/onnx/quantization/autotune/autotuner_base.pymodelopt/onnx/quantization/autotune/common.pymodelopt/onnx/quantization/autotune/insertion_points.py
🚧 Files skipped from review as they are similar to previous changes (1)
- modelopt/onnx/quantization/autotune/common.py
|
/ok to test d7f2df4 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1398 +/- ##
==========================================
- Coverage 76.92% 76.83% -0.09%
==========================================
Files 478 478
Lines 51440 51527 +87
==========================================
+ Hits 39568 39590 +22
- Misses 11872 11937 +65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Models with Conv->Relu->Concat patterns were not receiving Q/DQ nodes on Concat inputs, preventing TRT INT8 Concat fusion. Fixes: - Remove Concat from autotuner skip-ops list (Concat is byte-level copy in TRT and can pass INT8 data through) - Add Conv->[BN->]Add skip rule for TRT Conv+Add+Relu kernel fusion - Promote tensor-level Q/DQ when uncovered consumers are all Concat - Add atomic Concat-group mutation to scheme sampling (adaptive probability) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Will Guo <willg@nvidia.com>
Signed-off-by: Will Guo <willg@nvidia.com>
Signed-off-by: Will Guo <willg@nvidia.com>
d7f2df4 to
3073a3e
Compare
|
@gcunhase I have resolved one code-quality check issue, please help start a new CI job, thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/autotune/autotuner_base.py`:
- Around line 1015-1024: The Concat "add" branch in _mutate_insertion_points
currently appends points_to_add to the end which breaks canonical all_points
ordering; instead, after computing points_to_add (based on selected_keys and
complete_concat_groups[target]) merge selected_points and points_to_add and then
rebuild the final list by iterating all_points in order and including any point
whose (node_index,input_index) is in the merged selection set so the returned
list respects the canonical all_points ordering (use selected_points,
points_to_add, complete_concat_groups[target], and all_points to locate and
normalize).
🪄 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: d275fee3-023f-4a0c-a569-38861bc0fc5f
📒 Files selected for processing (3)
modelopt/onnx/quantization/autotune/autotuner_base.pymodelopt/onnx/quantization/autotune/common.pymodelopt/onnx/quantization/autotune/insertion_points.py
✅ Files skipped from review due to trivial changes (1)
- modelopt/onnx/quantization/autotune/common.py
🚧 Files skipped from review as they are similar to previous changes (1)
- modelopt/onnx/quantization/autotune/insertion_points.py
Signed-off-by: Will Guo <willg@nvidia.com>
Signed-off-by: Will Guo <willg@nvidia.com>
Signed-off-by: Will Guo <willg@nvidia.com>
Models with Conv->Relu->Concat patterns were not receiving Q/DQ nodes on Concat inputs, preventing TRT INT8 Concat fusion. Three fixes:
What does this PR do?
Summary
Usage
# Add a code snippet demonstrating how to use thisTesting
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
Summary by CodeRabbit