[OMNIML-5562] Add FAR3D ONNX PTQ and accuracy evaluation example#2012
[OMNIML-5562] Add FAR3D ONNX PTQ and accuracy evaluation example#2012ajrasane wants to merge 6 commits into
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a FAR3D ONNX post-training quantization example with a CUDA container, Argoverse 2 metadata and calibration preparation, INT8/FP8 quantization, TensorRT engine evaluation, and workflow documentation. ChangesFAR3D ONNX PTQ workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ValidationDataloader
participant Far3DPipeline
participant EncoderTensorRTEngine
participant DecoderTensorRTEngine
participant DatasetEvaluate
ValidationDataloader->>Far3DPipeline: dataset batch
Far3DPipeline->>EncoderTensorRTEngine: run image encoder
EncoderTensorRTEngine-->>Far3DPipeline: encoder outputs
Far3DPipeline->>DecoderTensorRTEngine: run stateful decoder
DecoderTensorRTEngine-->>Far3DPipeline: boxes, scores, labels, state outputs
Far3DPipeline->>DatasetEvaluate: prediction records
DatasetEvaluate-->>Far3DPipeline: dataset metrics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2012 +/- ##
==========================================
- Coverage 78.47% 75.74% -2.73%
==========================================
Files 518 518
Lines 58574 58574
==========================================
- Hits 45967 44368 -1599
- Misses 12607 14206 +1599
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:
|
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>
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-4-8) — DM the bot to share feedback.
New end-to-end FAR3D ONNX PTQ + Argoverse 2 eval example under examples/onnx_ptq/far3d/ (+923/-0, 10 files). The modelopt.onnx.quantization.quantize API usage checks out (verified args: quantize_mode, calibration_data_reader, calibration_eps, nodes_to_exclude, high_precision_dtype, output_path all match the current signature), the graph-exclusion BFS in find_encoder_nodes_to_exclude is sound ("downstream of lateral_convs" + all OSA4_5 nodes), and reusing modelopt.onnx.quantization.quantize rather than reinventing quantization is the right call (no duplicate subsystem introduced — the new code is example glue: TRT runners, calibration readers, dataset plumbing). No prompt-injection attempts in the untrusted content.
Flagging for human sign-off rather than approving because:
-
Licensing — requires human review (cannot auto-approve). This PR (a) edits the top-level
LICENSE(addsCopyright (c) OpenMMLab. All rights reserved.under the Apache-2.0 third-party section) and (b) copies/adapts external code:evaluate.pyis "Adapted from NVIDIA/DL4AGX … modified from megvii-research/Far3D", carrying an OpenMMLab copyright + "Modified by Zhiqi Li" stacked above the standard NVIDIA SPDX header. It also vendors a.patchthat modifies third-party FAR3D source. The attribution looks conscientious (correct license bucket inLICENSE, retained upstream notices), but license decisions have legal implications and need a human to sign off. Please confirm the combined third-party+NVIDIA header onevaluate.pyand the DL4AGX/Far3D provenance are acceptable perCONTRIBUTING.md. -
No tests, and no CI-exercisable path. Author marks tests N/A and the PR body mentions "synthetic calibration-reader and graph-exclusion tests" that aren't in the diff. Consistent with the repo's example-dir convention (no other onnx_ptq example ships unit tests), and the entire workflow is GPU/TensorRT/mmdet3d-only so it can't run in CPU CI. The one piece of self-contained, testable logic (
find_encoder_nodes_to_exclude) could reasonably get a small unit test if the team wants any coverage. -
CHANGELOG not updated (marked N/A). New examples are often exempt, but per repo convention new features get a CHANGELOG bullet — owner's call whether this example warrants one.
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
examples/onnx_ptq/far3d/evaluate.py (2)
257-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the conditional
mmcv.utilsimport to the top-level imports.This local import has no explanatory comment, and
mmcv(the same package) is already imported at the top of the file, so it isn't a genuinely circular, optional, or heavy dependency case.As per coding guidelines: "Keep imports at the top of Python source and test files; use local imports only for justified circular dependencies, optional dependencies, or unusually heavy imports, with a brief explanatory comment."
🤖 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 `@examples/onnx_ptq/far3d/evaluate.py` around lines 257 - 260, Move import_modules_from_strings from mmcv.utils into the top-level import section of evaluate.py, alongside the existing mmcv import, and remove the local import inside the cfg.get("custom_imports") conditional. Preserve the conditional invocation and its existing arguments.Source: Coding guidelines
59-142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider pre-allocating and reusing I/O buffers instead of reallocating on every call.
input_shapes/output_shapesare fixed once in__init__(static engine shapes) and never change, yet__call__callsaligned_tensor(...)for every input/output on every invocation — across the full 23,522-frame evaluation (encoder + decoder) plus calibration prep.self.statealready demonstrates the allocate-once/reuse pattern; the same approach could apply to the non-state input/output buffers, avoiding repeated CUDA allocations on this hot path.♻️ Sketch of the refactor
def __init__(self, engine_path, state_names=()): ... + self.input_buffers = { + name: aligned_tensor(shape, self.tensor_dtypes[name], "cuda") + for name, shape in self.input_shapes.items() + if name not in self.state + } + self.output_buffers = { + name: aligned_tensor(shape, self.tensor_dtypes[name], "cuda") + for name, shape in self.output_shapes.items() + } + for name, buf in {**self.input_buffers, **self.output_buffers}.items(): + self.context.set_tensor_address(name, buf.data_ptr()) def __call__(self, stream, **inputs): - input_buffers = {} for name, shape in self.input_shapes.items(): if name in self.state: continue value = self.prepare_input(name, inputs) - buffer = aligned_tensor(shape, value.dtype, value.device) - buffer.copy_(value) - input_buffers[name] = buffer - self.context.set_tensor_address(name, buffer.data_ptr()) - - outputs = {} - for name, shape in self.output_shapes.items(): - output = aligned_tensor(shape, self.tensor_dtypes[name], "cuda") - outputs[name] = output - self.context.set_tensor_address(name, output.data_ptr()) + self.input_buffers[name].copy_(value) if not self.context.execute_async_v3(stream.cuda_stream): raise RuntimeError("TensorRT execution failed") stream.synchronize() - return outputs + return self.output_buffers🤖 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 `@examples/onnx_ptq/far3d/evaluate.py` around lines 59 - 142, Pre-allocate reusable non-state input and output buffers in TensorRTRunner.__init__, using the fixed input_shapes, output_shapes, and tensor_dtypes, and bind their addresses once. Update __call__ to copy prepared inputs into the existing input buffers and reuse the existing output buffers instead of calling aligned_tensor for each invocation, while preserving state-buffer handling and returned outputs.examples/onnx_ptq/far3d/quantize.py (1)
76-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDefensive gap: empty/duplicate
node.namesilently truncates the exclusion BFS.
nodes_by_name[node.name] = nodeoverwrites on name collisions, and the BFS'sif not name: continue(line 94) drops any node with an empty name fromexcludedand stops traversing its outputs — so any accuracy-sensitive node reachable only through an unnamed node would be missed. Likely low-probability given this graph's nodes carry meaningful names (OSA4_5,lateral_convs), but worth a defensive guard since a silent miss here degrades accuracy without any error.🤖 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 `@examples/onnx_ptq/far3d/quantize.py` around lines 76 - 99, The find_encoder_nodes_to_exclude function must preserve BFS traversal through unnamed and duplicate-name nodes instead of indexing nodes solely by node.name. Track graph nodes and their outputs using stable node identities or an adjacency structure that does not overwrite collisions, include unnamed nodes in traversal, and only use names when adding valid exclusions while continuing through every reachable node.examples/onnx_ptq/far3d/Dockerfile (1)
1-81: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRoot user (Trivy DS-0002).
No
USERdirective; container runs as root. This matches other example Dockerfiles in the repo and the README's documented--privileged --gpus=allworkflow, so the practical risk is low for a local example build.🤖 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 `@examples/onnx_ptq/far3d/Dockerfile` around lines 1 - 81, Add a non-root runtime user and switch to it with a USER directive in the Dockerfile, ensuring the user can access and execute the installed /opt/far3d and /opt/modelopt contents. Preserve the existing dependency installation steps, which must continue running as root before the user switch.Source: Linters/SAST tools
🤖 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 `@examples/onnx_ptq/far3d/quantize.py`:
- Around line 118-133: The nodes passed by quantize_encoder through
nodes_to_exclude must match only the exact excluded node names. Transform the
result of find_encoder_nodes_to_exclude into escaped, fully anchored regex
patterns before supplying it to quantize, preserving the existing exclusion set
without allowing prefix or metacharacter-based matches.
---
Nitpick comments:
In `@examples/onnx_ptq/far3d/Dockerfile`:
- Around line 1-81: Add a non-root runtime user and switch to it with a USER
directive in the Dockerfile, ensuring the user can access and execute the
installed /opt/far3d and /opt/modelopt contents. Preserve the existing
dependency installation steps, which must continue running as root before the
user switch.
In `@examples/onnx_ptq/far3d/evaluate.py`:
- Around line 257-260: Move import_modules_from_strings from mmcv.utils into the
top-level import section of evaluate.py, alongside the existing mmcv import, and
remove the local import inside the cfg.get("custom_imports") conditional.
Preserve the conditional invocation and its existing arguments.
- Around line 59-142: Pre-allocate reusable non-state input and output buffers
in TensorRTRunner.__init__, using the fixed input_shapes, output_shapes, and
tensor_dtypes, and bind their addresses once. Update __call__ to copy prepared
inputs into the existing input buffers and reuse the existing output buffers
instead of calling aligned_tensor for each invocation, while preserving
state-buffer handling and returned outputs.
In `@examples/onnx_ptq/far3d/quantize.py`:
- Around line 76-99: The find_encoder_nodes_to_exclude function must preserve
BFS traversal through unnamed and duplicate-name nodes instead of indexing nodes
solely by node.name. Track graph nodes and their outputs using stable node
identities or an adjacency structure that does not overwrite collisions, include
unnamed nodes in traversal, and only use names when adding valid exclusions
while continuing through every reachable node.
🪄 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: 85ac9fde-ebb2-416e-9794-10fdd8a25573
📒 Files selected for processing (10)
.pre-commit-config.yamlLICENSEexamples/onnx_ptq/README.mdexamples/onnx_ptq/far3d/Dockerfileexamples/onnx_ptq/far3d/README.mdexamples/onnx_ptq/far3d/evaluate.pyexamples/onnx_ptq/far3d/far3d_pytorch_25_06.patchexamples/onnx_ptq/far3d/prepare_calibration.pyexamples/onnx_ptq/far3d/prepare_metadata.pyexamples/onnx_ptq/far3d/quantize.py
| def quantize_encoder(args): | ||
| encoder_dir = Path(args.calibration_dir) | ||
| if (encoder_dir / "encoder").is_dir(): | ||
| encoder_dir /= "encoder" | ||
| excluded_nodes = find_encoder_nodes_to_exclude(args.encoder_onnx) | ||
| print(f"Excluding {len(excluded_nodes)} accuracy-sensitive nodes from quantization") | ||
| quantize( | ||
| onnx_path=args.encoder_onnx, | ||
| quantize_mode=args.quantization_mode, | ||
| calibration_data_reader=EncoderCalibrationReader(encoder_dir), | ||
| calibration_method="max", | ||
| calibration_eps=["cuda:0", "cpu"], | ||
| nodes_to_exclude=excluded_nodes, | ||
| high_precision_dtype="fp16", | ||
| output_path=args.encoder_output, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby references.
git ls-files examples/onnx_ptq/far3d
wc -l examples/onnx_ptq/far3d/quantize.py
cat -n examples/onnx_ptq/far3d/quantize.py | sed -n '1,220p'
# Find local references to nodes_to_exclude / quantize docs.
rg -n "nodes_to_exclude|quantize\(" -S examples modelopt . 2>/dev/null | sed -n '1,200p'Repository: NVIDIA/Model-Optimizer
Length of output: 28996
🌐 Web query:
modelopt.onnx.quantization.quantize nodes_to_exclude regular expressions support
💡 Result:
In the NVIDIA Model Optimizer (modelopt) library, the nodes_to_exclude parameter in the modelopt.onnx.quantization.quantize function explicitly supports regular expressions [1][2][3]. When you provide a list of strings to nodes_to_exclude, the library treats these strings as patterns and uses them to identify and exclude matching node names from the quantization process [4][5]. Specifically, the library includes utility functions (such as expand_node_names_from_patterns) that resolve these regular expression patterns against the nodes present in the ONNX graph [5]. This functionality is consistent across both the Python API (modelopt.onnx.quantization.quantize) and the command-line interface (--nodes_to_exclude), where regular expression support is standard [4][6].
Citations:
- 1: https://nvidia.github.io/Model-Optimizer/reference/generated/modelopt.onnx.quantization.quantize.html
- 2: https://github.com/NVIDIA/Model-Optimizer/blob/7f5fd650/modelopt/onnx/quantization/quantize.py
- 3: https://github.com/NVIDIA/TensorRT-Model-Optimizer/blob/main/modelopt/onnx/quantization/quantize.py
- 4: https://nvidia.github.io/Model-Optimizer/guides/_onnx_quantization.html
- 5: https://github.com/NVIDIA/Model-Optimizer/blob/f34f488a/modelopt/onnx/quantization/graph_utils.py
- 6: https://github.com/NVIDIA/Model-Optimizer/blob/7f5fd650/modelopt/onnx/quantization/__main__.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the pattern-expansion helper used by ONNX quantization exclusions.
sed -n '1,260p' modelopt/onnx/quantization/graph_utils.py
printf '\n---\n'
sed -n '220,280p' modelopt/onnx/quantization/int4.py
printf '\n---\n'
sed -n '420,470p' modelopt/onnx/quantization/quantize.pyRepository: NVIDIA/Model-Optimizer
Length of output: 15290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact implementation of node-name pattern expansion.
rg -n "def expand_node_names_from_patterns|expand_node_names_from_patterns\(" modelopt/onnx -S
printf '\n---\n'
sed -n '260,360p' modelopt/onnx/quantization/graph_utils.py
printf '\n---\n'
sed -n '1,220p' modelopt/onnx/quantization/int4.pyRepository: NVIDIA/Model-Optimizer
Length of output: 12612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full helper implementation and surrounding callers.
sed -n '1000,1060p' modelopt/onnx/quantization/graph_utils.py
printf '\n---\n'
sed -n '230,255p' modelopt/onnx/quantization/int4.py
printf '\n---\n'
sed -n '180,255p' modelopt/onnx/quantization/fp8.pyRepository: NVIDIA/Model-Optimizer
Length of output: 7094
Anchor nodes_to_exclude to exact node names. modelopt.onnx.quantization resolves these values with re.match, so raw names behave like prefix regexes; Conv_1 can also match Conv_10, and regex metacharacters can change the match set. Escape and anchor the names before passing them in.
🤖 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 `@examples/onnx_ptq/far3d/quantize.py` around lines 118 - 133, The nodes passed
by quantize_encoder through nodes_to_exclude must match only the exact excluded
node names. Transform the result of find_encoder_nodes_to_exclude into escaped,
fully anchored regex patterns before supplying it to quantize, preserving the
existing exclusion set without allowing prefix or metacharacter-based matches.
2 participants
What does this PR do?
Type of change: new example
Adds an end-to-end FAR3D ONNX PTQ example under
examples/onnx_ptq/far3d. The example prepares Argoverse 2 validation metadata and calibration batches, quantizes the image encoder to INT8, builds TensorRT engines, and evaluates 3D object detection accuracy.It also provides a reproducible FAR3D runtime image, preserves accuracy-sensitive encoder layers in high precision, and supports temporal decoder state during evaluation.
Usage
Testing
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: ✅Additional Information
Summary by CodeRabbit