[6034518] Fix Conv->Relu->Concat Q/DQ insertion gap#1398
Open
willg-nv wants to merge 3 commits intoNVIDIA:mainfrom
Open
[6034518] Fix Conv->Relu->Concat Q/DQ insertion gap#1398willg-nv wants to merge 3 commits intoNVIDIA:mainfrom
willg-nv wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Concat-group-aware mutations to the ONNX quantization autotuner: new config option, Concat-group atomic add/remove mutation in scheme sampling, skip heuristics for Conv→(BN)→Add→Relu main-paths, and tensor-level promotion when remaining consumers are Concat nodes. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
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>
willg-nv
force-pushed
the
fix/autoqdq-concat-qdq-insertion
branch
from
dcd28ee to
563757d
Compare
Contributor
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
gcunhase
changed the title
gcunhase
reviewed
May 6, 2026
| minimum_schemes_to_mutate: int = 10 | ||
| maximum_mutations: int = 3 | ||
| maximum_generation_attempts: int = 100 | ||
| concat_group_min_samples: int = 5 # Minimum Concat-group-aware mutations per region |
Contributor
There was a problem hiding this comment.
Why was 5 chosen here? What are the benefits or a larger or smaller number? @willg-nv
Contributor
Author
There was a problem hiding this comment.
The purpose of this change is to mutate insertion spec to insert Q/DQ for all concat input. But this probability should not be too high (5%) to avoid impaction on normal mutation prcoess. But when sample per region is too small (eg: 30), the estimated sample count to trigger "all concat input QDQ" would be ~1, this is too small, sometimes cannot be triggered. so concat_group_min_samples sets the minimum sample count which could trigger "all concat input QDQ". I expect this case could be covered, but with small probability.
Contributor
There was a problem hiding this comment.
So is the 5 in concat_group_min_samples: int = 5 a percentage? If so, can we add that in the comment? Thanks!
Signed-off-by: Will Guo <willg@nvidia.com>
Contributor
|
/ok to test d7f2df4 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1398 +/- ##
==========================================
- Coverage 77.40% 76.70% -0.70%
==========================================
Files 476 476
Lines 51319 51408 +89
==========================================
- Hits 39721 39434 -287
- Misses 11598 11974 +376
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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