Centralize 'trtexec' subprocess runs in ONNX into a single function#1268
Centralize 'trtexec' subprocess runs in ONNX into a single function#1268gcunhase wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Signed-off-by: gcunhase <4861122+gcunhase@users.noreply.github.com>
📝 WalkthroughWalkthroughRefactored subprocess execution logic by creating a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| def _run_trtexec(cmd, timeout=None): | ||
| """Run a 'trtexec' command via subprocess.""" | ||
| # Ensure that this command is a trtexec run | ||
| assert any("trtexec" in c for c in cmd), "Subprocess can only execute 'trtexec' commands" |
There was a problem hiding this comment.
@kevalmorabia97 please let me know if this is enough to ensure only trtexec runs or if this is even necessary. Thanks.
There was a problem hiding this comment.
Can we not pass trtexec in cmd and only pass arguments to trtexec so this function prepends trtexec to the cmd?
Please add type annotation also
gcunhase
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelopt/onnx/quantization/ort_utils.py`:
- Around line 47-53: Replace the fragile assert and the banned "# nosec" in
_run_trtexec: validate that cmd is a non-empty sequence and that cmd[0] contains
"trtexec" (or equals "trtexec") using an explicit runtime check (e.g., if not
cmd or "trtexec" not in str(cmd[0]): raise ValueError("...")), then call
subprocess.run(cmd, capture_output=True, text=True, timeout=timeout) without any
"# nosec" comment; keep the function name _run_trtexec and parameters (cmd,
timeout) and raise a clear exception (ValueError/RuntimeError) when validation
fails so Bandit checks are not bypassed.
🪄 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: Pro Plus
Run ID: 7e439fa2-8582-4851-9734-1eec0c8351c0
📒 Files selected for processing (2)
modelopt/onnx/quantization/autotune/benchmark.pymodelopt/onnx/quantization/ort_utils.py
| def _run_trtexec(cmd, timeout=None): | ||
| """Run a 'trtexec' command via subprocess.""" | ||
| # Ensure that this command is a trtexec run | ||
| assert any("trtexec" in c for c in cmd), "Subprocess can only execute 'trtexec' commands" | ||
|
|
||
| # Run trtexec command | ||
| return subprocess.run(cmd, capture_output=True, text=True, timeout=timeout) # nosec B603 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the problematic patterns exist at the expected locations.
rg -n 'def _run_trtexec|assert any\("trtexec"|# nosec B603|subprocess\.run\(' modelopt/onnx/quantization/ort_utils.py -C2Repository: NVIDIA/Model-Optimizer
Length of output: 452
🏁 Script executed:
# Check for calls to _run_trtexec to understand usage context
rg -n '_run_trtexec' modelopt/onnx/quantization/ort_utils.py -A 5 -B 5Repository: NVIDIA/Model-Optimizer
Length of output: 927
🏁 Script executed:
# Check if there are any imports of subprocess and how it's being used elsewhere
rg -n 'import subprocess|from subprocess' modelopt/onnx/quantization/ort_utils.py -B 2 -A 2Repository: NVIDIA/Model-Optimizer
Length of output: 184
🏁 Script executed:
# Verify the file structure and get full context around the function
sed -n '1,60p' modelopt/onnx/quantization/ort_utils.pyRepository: NVIDIA/Model-Optimizer
Length of output: 2324
Replace the assert gate and remove the # nosec bypass in _run_trtexec.
Line 50 uses assert for command validation, which can be disabled with Python optimization flags (-O, -OO), making it an unreliable security control. Line 53 adds # nosec B603, which violates repo policy: "Bandit security checks are enforced via pre-commit hooks. # nosec comments are not allowed as bypasses." Replace the assert with an explicit runtime check on cmd[0] and remove the nosec suppression. Request formal codeowner approval only if the subprocess pattern genuinely cannot be restructured to satisfy Bandit without exceptions.
Proposed fix
-def _run_trtexec(cmd, timeout=None):
+def _run_trtexec(cmd: Sequence[str], timeout: float | None = None):
"""Run a 'trtexec' command via subprocess."""
- # Ensure that this command is a trtexec run
- assert any("trtexec" in c for c in cmd), "Subprocess can only execute 'trtexec' commands"
+ if not cmd:
+ raise ValueError("Empty command passed to _run_trtexec")
+ exe = os.path.basename(str(cmd[0])).lower()
+ if exe not in {"trtexec", "trtexec.exe"}:
+ raise ValueError("Subprocess can only execute 'trtexec' commands")
# Run trtexec command
- return subprocess.run(cmd, capture_output=True, text=True, timeout=timeout) # nosec B603
+ return subprocess.run(list(cmd), capture_output=True, text=True, timeout=timeout, check=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelopt/onnx/quantization/ort_utils.py` around lines 47 - 53, Replace the
fragile assert and the banned "# nosec" in _run_trtexec: validate that cmd is a
non-empty sequence and that cmd[0] contains "trtexec" (or equals "trtexec")
using an explicit runtime check (e.g., if not cmd or "trtexec" not in
str(cmd[0]): raise ValueError("...")), then call subprocess.run(cmd,
capture_output=True, text=True, timeout=timeout) without any "# nosec" comment;
keep the function name _run_trtexec and parameters (cmd, timeout) and raise a
clear exception (ValueError/RuntimeError) when validation fails so Bandit checks
are not bypassed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1268 +/- ##
===========================================
+ Coverage 55.67% 76.52% +20.84%
===========================================
Files 458 459 +1
Lines 48464 48530 +66
===========================================
+ Hits 26982 37136 +10154
+ Misses 21482 11394 -10088
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:
|
There was a problem hiding this comment.
Can you unify with
alsoThere was a problem hiding this comment.
I was trying to avoid coupling modelopt/torch with modelopt/onnx. What do you think is best?
There was a problem hiding this comment.
modelopt.torch._deploy already has some dependencies on modelopt.onnx so its fine. We can keep this in modelopt.onnx.trt_utils or whatever names makes sense
For example this: https://github.com/NVIDIA/Model-Optimizer/blob/07ae8e71281a1c17898991b0cd76db5788f490bb/modelopt/torch/_deploy/_runtime/trt_client.py
There was a problem hiding this comment.
Good point, will look into this today.
2 participants
What does this PR do?
Type of change: code improvement
Subprocess is needed to run
trtexeccommands but those requirenosecapproval. This PR centralizes it into a single function for the ONNX workflow to avoid future approval triggers. The torch workflow has that centralized inrun_command.Original request: #1259 (comment)
Usage
Testing
N/A
Before your PR is "Ready for review"
CONTRIBUTING.md: N/ASummary by CodeRabbit