Repository navigation
Use trtexec_safe on safety platforms when using remoteAutoTuning - #1378
dthienan-nv wants to merge 58 commits into
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughReplaces a single latency regex with separate safe and standard patterns; enforces ssh:// for --remoteAutoTuningConfig and stores SSH credentials/remote paths; SCPs engines and runs remote trtexec_safe with fallback to trtexec --safe; adds subprocess timeouts and returns inf on failures. Changes
Sequence DiagramsequenceDiagram
participant Local as Local System
participant CLI as Arg Parser
participant Bench as TrtExecBenchmark
participant Remote as Remote Host
participant Parser as Latency Parser
Local->>CLI: Parse args (--remoteAutoTuningConfig, --safe)
CLI->>Bench: Provide parsed config
Bench->>Bench: Persist SSH creds, host, port, remote paths
alt remote autotuning && safe
Bench->>Bench: Build TensorRT engine
Bench->>Remote: scp engine -> remote_exec_path (timeout)
Remote-->>Bench: scp result
Bench->>Remote: ssh run `LD_LIBRARY_PATH=... trtexec_safe --loadEngine=...` (timeout)
alt safe stdout parsed
Remote-->>Bench: stdout
Bench->>Parser: parse with safe_pattern
else trtexec_safe fail
Bench->>Remote: ssh run `trtexec --safe --loadEngine=...` (timeout fallback)
Remote-->>Bench: stdout
Bench->>Parser: parse with std_pattern
end
else local or non-safe
Bench->>Bench: run local trtexec (timeout)
Bench->>Parser: parse with std_pattern
end
Parser-->>Bench: latency or parse-failure -> return latency or inf
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/autotune/benchmark.py`:
- Line 263: The code assumes required query params exist and directly indexes
self.remote_options["remote_exec_path"] (and later "remote_lib_path"), which can
raise a KeyError; fix by validating that both "remote_exec_path" and
"remote_lib_path" are present in self.remote_options before using them (e.g., in
the constructor or the method that parses remote options), and raise a clear
ValueError or return a helpful error message if missing; after validation,
continue to compute self.remote_bin_path =
os.path.dirname(str(self.remote_options["remote_exec_path"])) and the remote lib
path usage as before.
- Around line 355-378: The code in benchmark.py (scp_cmd, trtexec_safe_cmd,
subprocess.run calls) has multiple security and correctness issues: remove all
"# nosec" comments, validate self.remote_port (ensure not None and use the
correct ssh/scp port flags), never embed self.remote_password into command
arguments, and stop passing a single shell-interpreted string to SSH
(trtexec_safe_cmd) which risks command injection from self.remote_engine_path,
self.remote_options['remote_lib_path'], and self.remote_bin_path. Fix by using
key-based SSH authentication (or only use sshpass with explicit approver
consent), build scp_cmd and ssh target as "user@host" (no password), pass port
via the correct flag, and pass remote commands/LD_LIBRARY_PATH safely: either
set environment via subprocess.run(env=...) when running locally or construct a
safely-escaped remote command using shlex.quote for each component (or send a
prepared remote script) and invoke subprocess.run with a list of args (not a
single concatenated shell string); also check subprocess.returncode and capture
output/errors for logging. Ensure changes touch scp_cmd, trtexec_safe_cmd,
subprocess.run usages, and validations for
remote_port/remote_user/remote_engine_path.
- Around line 291-293: The info log in the Benchmark (or surrounding
class/method where self._base_cmd is built and logged) prints the full base
command which can leak credentials from arguments like --remoteAutoTuningConfig
(e.g., ssh://user:password@host); before calling self.logger.info, create a
redacted copy of self._base_cmd that masks sensitive values by detecting args
that contain remoteAutoTuningConfig or ssh:// patterns (or args starting with
"--remoteAutoTuningConfig") and replace the password portion (or entire URI)
with a placeholder such as "ssh://user:****@host" or "[REDACTED]"; then log the
redacted command instead of the original to avoid credential exposure.
🪄 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: b8d2a6d0-2a7c-4a27-9841-5b576f1c8ec2
📒 Files selected for processing (1)
modelopt/onnx/quantization/autotune/benchmark.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/autotune/benchmark.py`:
- Around line 293-296: The remote-config cleanup currently only removes the
"--remoteAutoTuningConfig" token from trtexec_args but leaves the following URL
token and doesn't clear the self.has_remote_config flag, so update the cleanup
in the ImportError path to remove both the flag and its following token when
present and set self.has_remote_config = False; specifically, process the
trtexec_args list in the block that handles ImportError to filter out
"--remoteAutoTuningConfig" and the next element (if any) and then set
self.has_remote_config = False so subsequent checks of
self.is_safe/self.has_remote_config behave correctly.
- Around line 376-404: The SCP/SSH subprocess.run calls (scp_cmd and the
trtexec_safe_cmd invocations) lack timeouts and can hang; update each
subprocess.run that executes scp_cmd and trtexec_safe_cmd to pass a configurable
timeout (e.g., self.ssh_timeout or a new attribute like self.remote_timeout with
a sensible default) and handle subprocess.TimeoutExpired by killing the
operation, logging an error via self.logger.error, and returning float("inf")
(same failure path as other errors). Apply this to the three subprocess.run
sites shown (the initial scp_cmd run, the first trtexec_safe_cmd run, and the
fallback trtexec_safe_cmd run), using the timeout kwarg and wrapping calls in
try/except subprocess.TimeoutExpired to ensure deterministic autotune behavior.
- Around line 256-261: The code assigns parsed.username/hostname directly to
self.remote_user and self.remote_ip, which allows None to overwrite the intended
defaults and permits missing hostnames; update the assignment logic in the
constructor/initializer that sets self.remote_user, self.remote_password,
self.remote_ip, and self.remote_port so that parsed.username only replaces the
default (e.g., "root") when not None/empty, parsed.password is handled
similarly, and parsed.hostname is validated—if hostname is missing raise a
ValueError (or similar) with a clear message; keep the fallback for remote_port
to 22 when parsed.port is None.
🪄 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: 05684e65-2b3d-4d9b-aa78-493751fc56b9
📥 Commits
Reviewing files that changed from the base of the PR and between 19a4961 and be947cf7c2232344a8c47b8707f662cbe3648ea7.
📒 Files selected for processing (1)
modelopt/onnx/quantization/autotune/benchmark.py
There was a problem hiding this comment.
♻️ Duplicate comments (4)
modelopt/onnx/quantization/autotune/benchmark.py (4)
294-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImportError cleanup leaves remote state inconsistent for split-arg form.
When args are passed as
--remoteAutoTuningConfig <url>, this only removes the flag token and leaves the URL behind.self.has_remote_configalso remainsTrue, so remote-safe flow can still be entered later.Suggested fix
- trtexec_args = [ - arg for arg in trtexec_args if "--remoteAutoTuningConfig" not in arg - ] + filtered_args: list[str] = [] + skip_next = False + for arg in trtexec_args: + if skip_next: + skip_next = False + continue + if arg == "--remoteAutoTuningConfig": + skip_next = True + continue + if arg.startswith("--remoteAutoTuningConfig="): + continue + filtered_args.append(arg) + trtexec_args = filtered_args + self.has_remote_config = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 294 - 297, The removal of "--remoteAutoTuningConfig" only deletes the flag token but leaves the URL token and doesn't clear self.has_remote_config; update the cleanup for trtexec_args in the code around trtexec_args and self.is_safe so it removes both the flag and its following value when the flag appears in split form (e.g., iterate through trtexec_args, drop "--remoteAutoTuningConfig" and the next token), and ensure self.has_remote_config is set to False when no remote config token remains so the remote-safe flow won't be entered erroneously.
339-341:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
# nosecbypasses on subprocess calls violate repository security policy.These inline Bandit bypass comments are not allowed by the project policy and should be removed before merge (with explicit safety justification and required reviewer approval handled in PR process if needed).
As per coding guidelines: "Any use of '# nosec' comments to bypass Bandit security checks is not allowed."
Also applies to: 379-381, 395-400, 414-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 339 - 341, Remove the "# nosec" comments on the subprocess.run calls (the one creating result = subprocess.run(..., timeout=self.remote_timeout_sec) and the other occurrences at the referenced ranges) and replace the bypass with secure handling: ensure each call passes a validated list of arguments (no shell=True), validate or sanitize the cmd input before use, handle subprocess.TimeoutExpired and CalledProcessError exceptions and log/propagate failures instead of silencing them, and ensure capture_output/text remain intentional; update the code around the subprocess.run calls (refer to the result = subprocess.run(...) invocation and the other similar subprocess.run sites) to implement these safe patterns.
257-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve defaults and validate SSH authority fields before building remote targets.
parsed.username/parsed.hostnamecan beNone; assigning directly here can produce invalid endpoints (e.g.,None@None) and hides the intended"root"default user fallback.Suggested fix
- self.remote_user = parsed.username - self.remote_password = parsed.password - self.remote_ip = parsed.hostname - self.remote_port = parsed.port - if self.remote_port is None: - self.remote_port = 22 + self.remote_user = parsed.username or self.remote_user + self.remote_password = parsed.password or "" + self.remote_ip = parsed.hostname + if not self.remote_ip: + raise ValueError( + "Missing hostname in --remoteAutoTuningConfig (expected ssh://user[:pass]@host[:port]?...)." + ) + self.remote_port = parsed.port or 22🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 257 - 262, Do not blindly overwrite SSH fields with possibly-None parsed values; instead validate and preserve defaults: only set self.remote_user = parsed.username if parsed.username is not None (otherwise keep existing default "root"), only set self.remote_password = parsed.password if parsed.password is not None, only set self.remote_ip = parsed.hostname if parsed.hostname is not None (and raise or handle missing hostname), and set self.remote_port = parsed.port if parsed.port is not None else keep default 22; ensure the code uses these validated attributes when constructing remote targets so you never get "None@None".
368-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
sshpass -pbecause it exposes the password in process arguments.Passing secrets via command-line arguments can leak through process listings. Prefer SSH keys, or use
sshpass -ewithSSHPASSset in a transient env passed tosubprocess.run.As per coding guidelines: "Never hardcode or log credentials/tokens/passwords; ensure error/log messages do not expose secrets."
Does `sshpass -p <password>` expose credentials via process listings, and is `sshpass -e` (SSHPASS env var) the recommended safer alternative?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 368 - 370, The current code appends "sshpass", "-p", and self.remote_password to ssh_pass (in the block building the SSH command), which exposes the password via process arguments; change this to use "sshpass", "-e" and remove the "-p" and self.remote_password from ssh_pass, then pass the password via a transient environment variable SSHPASS when invoking subprocess.run/call (add/merge an env dict with {"SSHPASS": self.remote_password} or use an existing env copy) so the secret is not visible in argv; also prefer using SSH keys if available and remove any logging of self.remote_password in functions building or executing the SSH command (e.g., the code that constructs ssh_pass and the caller that runs subprocess).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 294-297: The removal of "--remoteAutoTuningConfig" only deletes
the flag token but leaves the URL token and doesn't clear
self.has_remote_config; update the cleanup for trtexec_args in the code around
trtexec_args and self.is_safe so it removes both the flag and its following
value when the flag appears in split form (e.g., iterate through trtexec_args,
drop "--remoteAutoTuningConfig" and the next token), and ensure
self.has_remote_config is set to False when no remote config token remains so
the remote-safe flow won't be entered erroneously.
- Around line 339-341: Remove the "# nosec" comments on the subprocess.run calls
(the one creating result = subprocess.run(..., timeout=self.remote_timeout_sec)
and the other occurrences at the referenced ranges) and replace the bypass with
secure handling: ensure each call passes a validated list of arguments (no
shell=True), validate or sanitize the cmd input before use, handle
subprocess.TimeoutExpired and CalledProcessError exceptions and log/propagate
failures instead of silencing them, and ensure capture_output/text remain
intentional; update the code around the subprocess.run calls (refer to the
result = subprocess.run(...) invocation and the other similar subprocess.run
sites) to implement these safe patterns.
- Around line 257-262: Do not blindly overwrite SSH fields with possibly-None
parsed values; instead validate and preserve defaults: only set self.remote_user
= parsed.username if parsed.username is not None (otherwise keep existing
default "root"), only set self.remote_password = parsed.password if
parsed.password is not None, only set self.remote_ip = parsed.hostname if
parsed.hostname is not None (and raise or handle missing hostname), and set
self.remote_port = parsed.port if parsed.port is not None else keep default 22;
ensure the code uses these validated attributes when constructing remote targets
so you never get "None@None".
- Around line 368-370: The current code appends "sshpass", "-p", and
self.remote_password to ssh_pass (in the block building the SSH command), which
exposes the password via process arguments; change this to use "sshpass", "-e"
and remove the "-p" and self.remote_password from ssh_pass, then pass the
password via a transient environment variable SSHPASS when invoking
subprocess.run/call (add/merge an env dict with {"SSHPASS":
self.remote_password} or use an existing env copy) so the secret is not visible
in argv; also prefer using SSH keys if available and remove any logging of
self.remote_password in functions building or executing the SSH command (e.g.,
the code that constructs ssh_pass and the caller that runs subprocess).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cb68d482-2dee-449b-ba0a-2c4b8e306e3a
📥 Commits
Reviewing files that changed from the base of the PR and between be947cf7c2232344a8c47b8707f662cbe3648ea7 and 28e4e8e8438e00c902e9aa2543807acc758b484f.
📒 Files selected for processing (1)
modelopt/onnx/quantization/autotune/benchmark.py
There was a problem hiding this comment.
♻️ Duplicate comments (3)
modelopt/onnx/quantization/autotune/benchmark.py (3)
297-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact
--remoteAutoTuningConfigbefore logging command templateAt Line 299, logging the full command can leak credentials from SSH URLs in remote autotuning config.
Proposed fix
- self.logger.debug(f"Base command template: {' '.join(self._base_cmd)}") + redacted_cmd: list[str] = [] + skip_next_remote_cfg_value = False + for arg in self._base_cmd: + if skip_next_remote_cfg_value: + skip_next_remote_cfg_value = False + redacted_cmd.append("[REDACTED_REMOTE_CONFIG]") + continue + if arg == "--remoteAutoTuningConfig": + redacted_cmd.append(arg) + skip_next_remote_cfg_value = True + continue + if arg.startswith("--remoteAutoTuningConfig="): + redacted_cmd.append("--remoteAutoTuningConfig=[REDACTED_REMOTE_CONFIG]") + continue + redacted_cmd.append(arg) + self.logger.debug(f"Base command template: {' '.join(redacted_cmd)}")Based on learnings: “Avoid logging sensitive inputs, paths, tokens, proprietary model details, or overly verbose operational information that could leak security-relevant data.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 297 - 299, The debug log prints the full command template via self._base_cmd (extended with trtexec_args), which can leak sensitive SSH URLs passed with the --remoteAutoTuningConfig flag; before calling self.logger.debug, create a sanitized copy of the command list (do not mutate self._base_cmd) and replace any argument that startswith "--remoteAutoTuningConfig=" or follows the "--remoteAutoTuningConfig" token with a redacted placeholder like "--remoteAutoTuningConfig=<REDACTED>" and then log ' '.join(sanitized_cmd) in the logger.debug call so the sensitive value is not exposed (keep use of self._base_cmd, trtexec_args, and self.logger.debug).
257-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate SSH authority fields before assigning remote target
On Line 257 and Line 259,
parsed.username/parsed.hostnamecan beNone. That can produce invalid targets and fail later with unclear errors.Proposed fix
- self.remote_user = parsed.username - self.remote_password = parsed.password - self.remote_ip = parsed.hostname - self.remote_port = parsed.port - if self.remote_port is None: - self.remote_port = 22 + self.remote_user = parsed.username or self.remote_user + self.remote_password = parsed.password or "" + self.remote_ip = parsed.hostname + if not self.remote_ip: + raise ValueError( + "Missing hostname in --remoteAutoTuningConfig " + "(expected ssh://user[:pass]@host[:port]?...)." + ) + self.remote_port = parsed.port or 22🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 257 - 262, Validate parsed.username and parsed.hostname before assigning to self.remote_user and self.remote_ip: if either is None, raise a clear ValueError (or custom exception) that includes the original target string and explains that SSH username and host are required; keep the existing fallback for parsed.port (set to 22 if None) and still assign parsed.password to self.remote_password (you may allow None for password but document it), and update the assignment block that sets self.remote_user, self.remote_password, self.remote_ip, self.remote_port to perform these checks and raise the informative error instead of silently producing invalid targets.
340-340:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove forbidden
# nosecsuppressions on subprocess calls
# nosecis still present on Line 340, Line 380, Line 399, and Line 418. This violates the repo’s Python security policy formodelopt/**/*.py.Proposed fix
- result = subprocess.run( - cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec - ) # nosec B603 + result = subprocess.run( + cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec + ) ... - result = subprocess.run( - scp_cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec - ) # nosec B603 + result = subprocess.run( + scp_cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec + ) ... - result = subprocess.run( - trtexec_safe_cmd, - capture_output=True, - text=True, - timeout=self.remote_timeout_sec, - ) # nosec B603 + result = subprocess.run( + trtexec_safe_cmd, + capture_output=True, + text=True, + timeout=self.remote_timeout_sec, + ) ... - result = subprocess.run( - trtexec_safe_cmd, - capture_output=True, - text=True, - timeout=self.remote_timeout_sec, - ) # nosec B603 + result = subprocess.run( + trtexec_safe_cmd, + capture_output=True, + text=True, + timeout=self.remote_timeout_sec, + )As per coding guidelines: “Any use of '# nosec' comments to bypass Bandit security checks is not allowed.”
Also applies to: 380-380, 399-399, 418-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` at line 340, Remove the forbidden "# nosec" suppressions on subprocess calls and fix the underlying security issue: locate the subprocess invocations (e.g., calls to subprocess.run / subprocess.Popen in benchmark.py) that currently have "# nosec B603", remove those comments, and make the calls secure by passing arguments as a list (not a single shell string), ensuring shell=False, sanitizing/validating any dynamic inputs before use (or using shlex.split on trusted strings), and properly handling exceptions/return codes; update the relevant call sites (the subprocess.run/subprocess.Popen usages) to follow these patterns so Bandit warnings are resolved without silencing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 297-299: The debug log prints the full command template via
self._base_cmd (extended with trtexec_args), which can leak sensitive SSH URLs
passed with the --remoteAutoTuningConfig flag; before calling self.logger.debug,
create a sanitized copy of the command list (do not mutate self._base_cmd) and
replace any argument that startswith "--remoteAutoTuningConfig=" or follows the
"--remoteAutoTuningConfig" token with a redacted placeholder like
"--remoteAutoTuningConfig=<REDACTED>" and then log ' '.join(sanitized_cmd) in
the logger.debug call so the sensitive value is not exposed (keep use of
self._base_cmd, trtexec_args, and self.logger.debug).
- Around line 257-262: Validate parsed.username and parsed.hostname before
assigning to self.remote_user and self.remote_ip: if either is None, raise a
clear ValueError (or custom exception) that includes the original target string
and explains that SSH username and host are required; keep the existing fallback
for parsed.port (set to 22 if None) and still assign parsed.password to
self.remote_password (you may allow None for password but document it), and
update the assignment block that sets self.remote_user, self.remote_password,
self.remote_ip, self.remote_port to perform these checks and raise the
informative error instead of silently producing invalid targets.
- Line 340: Remove the forbidden "# nosec" suppressions on subprocess calls and
fix the underlying security issue: locate the subprocess invocations (e.g.,
calls to subprocess.run / subprocess.Popen in benchmark.py) that currently have
"# nosec B603", remove those comments, and make the calls secure by passing
arguments as a list (not a single shell string), ensuring shell=False,
sanitizing/validating any dynamic inputs before use (or using shlex.split on
trusted strings), and properly handling exceptions/return codes; update the
relevant call sites (the subprocess.run/subprocess.Popen usages) to follow these
patterns so Bandit warnings are resolved without silencing them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 25d0d07d-dd26-491d-b62d-0b95541d748f
📥 Commits
Reviewing files that changed from the base of the PR and between 28e4e8e8438e00c902e9aa2543807acc758b484f and 36600920454ad8e5d3f5ca10b15c4e6a3007c055.
📒 Files selected for processing (1)
modelopt/onnx/quantization/autotune/benchmark.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modelopt/onnx/quantization/autotune/benchmark.py (2)
300-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential credential exposure in logs
Line 302 logs the full base command including
trtexec_args, which may contain--remoteAutoTuningConfig=ssh://user:password@host.... This logs credentials at debug level.Proposed fix: Redact credentials before logging
self._base_cmd.extend(trtexec_args) - self.logger.debug(f"Base command template: {' '.join(self._base_cmd)}") + # Redact credentials from log output + redacted_cmd = [ + re.sub(r"(ssh://[^:]*:)[^@]+(@)", r"\1****\2", arg) for arg in self._base_cmd + ] + self.logger.debug(f"Base command template: {' '.join(redacted_cmd)}")As per coding guidelines (SECURITY.md): "Don't log secrets/credentials/tokens/SSH details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 300 - 302, The debug log prints the full base command including trtexec_args which may contain SSH credentials (e.g., --remoteAutoTuningConfig=ssh://user:password@host); update the logging in the code path after self._base_cmd.extend(trtexec_args) so you sanitize trtexec_args (or a copy of self._base_cmd) before calling self.logger.debug. Implement a small redaction step in benchmark.py that detects and masks credential patterns like ssh://user:password@host or key=value pairs containing passwords/tokens (e.g., replace the password portion with **** or remove the value) and then log the sanitized command using the same logger.debug call instead of the raw command.
341-343:⚠️ Potential issue | 🔴 CriticalRemove
# nosec B603comments—they violate security guidelines without required justificationThe code contains four
# nosec B603comments (lines 343, 383, 402, 421) that bypass Bandit security checks. Per SECURITY.md and coding guidelines,# noseccomments are forbidden unless accompanied by:
- An inline comment explaining why the pattern is necessary and why it is safe in this specific context
- A PR description requesting review from
@NVIDIA/modelopt-setup-codeownerswith explicit justificationCurrently, neither is present. The subprocess calls themselves are safe (list arguments, no
shell=True, proper use ofshlex.quoteon user-controlled values), so removing the# noseccomments should allow Bandit to pass without lowering security posture.Fix: Remove # nosec comments from all four subprocess.run calls
result = subprocess.run( cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec - ) # nosec B603 + ) ... result = subprocess.run( scp_cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec - ) # nosec B603 + ) ... result = subprocess.run( trtexec_safe_cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec, - ) # nosec B603 + ) ... result = subprocess.run( trtexec_safe_cmd, capture_output=True, text=True, timeout=self.remote_timeout_sec, - ) # nosec B603 + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 341 - 343, Remove the forbidden "# nosec B603" annotations from the subprocess.run calls in this module (the four occurrences that call subprocess.run(..., timeout=self.remote_timeout_sec) and the other subprocess.run(...) invocations using list arguments and shlex.quote); leave the subprocess.run call signatures and argument lists unchanged. Do not add any other code or suppressions; if a justification is later required, include an inline safety comment plus the PR review request to `@NVIDIA/modelopt-setup-codeowners` per guidelines.
🧹 Nitpick comments (3)
modelopt/onnx/quantization/autotune/benchmark.py (3)
439-441: 💤 Low valueConsider adding specific timeout handling for better error messages
The generic
except Exceptionat line 439 catchessubprocess.TimeoutExpired, but the error message "Benchmark failed: {e}" doesn't clearly indicate a timeout occurred. A specific handler would improve debugging.Proposed fix
+ except subprocess.TimeoutExpired as e: + self.logger.error(f"Benchmark timed out after {self.remote_timeout_sec}s: {e}") + return float("inf") except Exception as e: self.logger.error(f"Benchmark failed: {e}") return float("inf")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 439 - 441, The generic except in the benchmarking routine (the block that currently does except Exception as e and calls self.logger.error(f"Benchmark failed: {e}") then returns float("inf")) should be split to handle subprocess.TimeoutExpired specifically: add an except subprocess.TimeoutExpired as e clause before the generic except to log a clear timeout message (e.g., self.logger.error(f"Benchmark timed out after {timeout}s: {e}")) and return float("inf"), then keep the broad except Exception as e for other failures; reference the existing self.logger.error usage and the function/method containing this try/except to locate where to add the new except.
256-265: 💤 Low valueValidate empty username in addition to None
urlparse("ssh://:password@host")returnsusername=""which passes theis Nonecheck but produces an invalid SSH target. Consider validating that the username is non-empty.Proposed fix
self.remote_user = parsed.username self.remote_password = parsed.password self.remote_ip = parsed.hostname self.remote_port = parsed.port - if self.remote_user is None: + if not self.remote_user: raise ValueError("Unable to parse remote user from --remoteAutoTuningConfig")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 256 - 265, After parsing the SSH URL into parsed, tighten validation for parsed fields by ensuring self.remote_user is non-empty (not None and not an empty string) and likewise ensure self.remote_ip is a non-empty value; replace the existing "if self.remote_user is None" and "if self.remote_ip is None" checks with truthy checks (e.g., if not self.remote_user / if not self.remote_ip) so that cases like urlparse("ssh://:password@host") are rejected, while leaving the remote_port defaulting behavior (set to 22 when None) unchanged.
286-286: 💤 Low valueRedundant
is_safeassignmentLine 286 sets
self.is_safe = Trueinside the conditional block, but line 299 unconditionally reassignsself.is_safe = "--safe" in trtexec_argsimmediately after the block. Line 286 is redundant.Proposed fix
self.trtexec_args.append("--safe") - self.is_safe = TrueAlso applies to: 299-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/quantization/autotune/benchmark.py` at line 286, The assignment self.is_safe = True inside the conditional is redundant because self.is_safe is immediately overwritten later with self.is_safe = "--safe" in trtexec_args; in the class/method where trtexec args are prepared (look for the method that builds trtexec_args and sets self.is_safe in modelopt/onnx/quantization/autotune/benchmark.py), remove the earlier self.is_safe = True so the flag is set only once by the final assignment, leaving any conditional logic that affects trtexec_args intact.
🤖 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/autotune/benchmark.py`:
- Around line 292-297: The warning message inside the except ImportError block
in benchmark.py (the "except ImportError as e" handler that currently calls
self.logger.warning(...)) is misleading because it says
"--remoteAutoTuningConfig" will be removed but the code immediately raises the
exception; update the log and error handling in that except block of the
benchmarking logic (reference: the except ImportError as e handler and
self.logger) so the message accurately reflects that remote autotuning is
unsupported and the operation will abort: either change the log to a clear
error/exception-level message (e.g., use self.logger.error or
self.logger.exception) that includes e (the exception details) and then
re-raise, or, if you intend to continue by removing the arg, implement the
actual removal logic for "--remoteAutoTuningConfig" before proceeding; do not
leave the old "Removing --remoteAutoTuningConfig" text while still raising.
---
Duplicate comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 300-302: The debug log prints the full base command including
trtexec_args which may contain SSH credentials (e.g.,
--remoteAutoTuningConfig=ssh://user:password@host); update the logging in the
code path after self._base_cmd.extend(trtexec_args) so you sanitize trtexec_args
(or a copy of self._base_cmd) before calling self.logger.debug. Implement a
small redaction step in benchmark.py that detects and masks credential patterns
like ssh://user:password@host or key=value pairs containing passwords/tokens
(e.g., replace the password portion with **** or remove the value) and then log
the sanitized command using the same logger.debug call instead of the raw
command.
- Around line 341-343: Remove the forbidden "# nosec B603" annotations from the
subprocess.run calls in this module (the four occurrences that call
subprocess.run(..., timeout=self.remote_timeout_sec) and the other
subprocess.run(...) invocations using list arguments and shlex.quote); leave the
subprocess.run call signatures and argument lists unchanged. Do not add any
other code or suppressions; if a justification is later required, include an
inline safety comment plus the PR review request to
`@NVIDIA/modelopt-setup-codeowners` per guidelines.
---
Nitpick comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 439-441: The generic except in the benchmarking routine (the block
that currently does except Exception as e and calls
self.logger.error(f"Benchmark failed: {e}") then returns float("inf")) should be
split to handle subprocess.TimeoutExpired specifically: add an except
subprocess.TimeoutExpired as e clause before the generic except to log a clear
timeout message (e.g., self.logger.error(f"Benchmark timed out after {timeout}s:
{e}")) and return float("inf"), then keep the broad except Exception as e for
other failures; reference the existing self.logger.error usage and the
function/method containing this try/except to locate where to add the new
except.
- Around line 256-265: After parsing the SSH URL into parsed, tighten validation
for parsed fields by ensuring self.remote_user is non-empty (not None and not an
empty string) and likewise ensure self.remote_ip is a non-empty value; replace
the existing "if self.remote_user is None" and "if self.remote_ip is None"
checks with truthy checks (e.g., if not self.remote_user / if not
self.remote_ip) so that cases like urlparse("ssh://:password@host") are
rejected, while leaving the remote_port defaulting behavior (set to 22 when
None) unchanged.
- Line 286: The assignment self.is_safe = True inside the conditional is
redundant because self.is_safe is immediately overwritten later with
self.is_safe = "--safe" in trtexec_args; in the class/method where trtexec args
are prepared (look for the method that builds trtexec_args and sets self.is_safe
in modelopt/onnx/quantization/autotune/benchmark.py), remove the earlier
self.is_safe = True so the flag is set only once by the final assignment,
leaving any conditional logic that affects trtexec_args intact.
🪄 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: 05a6afb2-73a0-4d73-a098-c67808a0e42a
📥 Commits
Reviewing files that changed from the base of the PR and between 36600920454ad8e5d3f5ca10b15c4e6a3007c055 and 2f75b5ddd96f307bf822ff07b3dd22b45d4966c5.
📒 Files selected for processing (1)
modelopt/onnx/quantization/autotune/benchmark.py
There was a problem hiding this comment.
Code Review Findings
Critical
1. Existing mocked tests will fail (regression).
tests/gpu/onnx/quantization/autotune/test_benchmark.py:196 and :214 mock subprocess.run to return "[I] Latency: min = ... median = X ms" and assert bench.run(...) == X. The PR replaces latency_pattern with r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms", which doesn't match the "[I] Latency:" mocks. Running the tests against this PR's benchmark.py confirms it:
FAILED tests/gpu/onnx/quantization/autotune/test_benchmark.py::test_trtexec_run_returns_parsed_latency
FAILED tests/gpu/onnx/quantization/autotune/test_benchmark.py::test_trtexec_run_accepts_bytes_input
Fix: update the mock stdout in those two tests to use the new [I] GPU Compute Time: min = ..., max = ..., median = X ms format.
2. No tests cover any of the new logic.
133 lines of new code — URL parsing, query-param validation, port/user/host fallbacks, scp/ssh invocation, primary-vs-fallback remote command selection, two distinct latency regexes — and zero tests added or updated. At minimum, please add unit tests with subprocess.run mocked for:
--remoteAutoTuningConfigparsing happy-path (both=URLandURLarg forms).- Each
ValueErrorbranch in the parser (missing value, non-ssh://, missing user/host, missing query params, duplicate config arg). - The
safe_patternregex against a realisticAverage over N runs - GPU latency: X msline. - The
trtexec_safe→trtexec --safefallback path (returncode-based selection oflatency_pattern).
Important
3. # nosec B603 comments remain unresolved per SECURITY.md.
Per project security guidelines, # nosec is forbidden without @NVIDIA/modelopt-setup-codeowners approval and explicit PR-description justification. CodeRabbit raised this twice; the author replied "Partially addressed" but the four # nosec B603 comments at the new subprocess.run sites are still in benchmark.py (all on the new ssh/scp calls). Either remove them and seek codeowner approval with justification in the PR body, or align with whatever approved pattern the project uses for subprocess invocations.
4. FileNotFoundError handler misattributes the error.
The outer except FileNotFoundError in run() catches errors from any of the new subprocess calls (scp, ssh, sshpass) and logs:
trtexec binary not found: {self.trtexec_path}
Please ensure TensorRT is installed and trtexec path is correct
A missing sshpass or scp (likely on minimal containers) will produce that misleading message. Either differentiate by logging the actual e.filename, or scope the existing handler narrowly to the local trtexec call and add a separate handler for the remote path.
5. self.is_safe is set in two places; the inner assignment is dead.
self.is_safe = True inside the if "--safe" not in trtexec_args: block is followed by an unconditional self.is_safe = "--safe" in trtexec_args after the block (and the inner self.trtexec_args.append("--safe") mutates the same list trtexec_args aliases, so the outer assignment ends up True anyway). The inner one isn't a bug but is confusing — please drop it so there's a single source of truth.
6. Multi-valued query params silently corrupt remote paths.
self.remote_options = {
k: v[0] if len(v) == 1 else v for k, v in parse_qs(parsed.query).items()
}If remote_exec_path appears more than once in the URL (?remote_exec_path=a&remote_exec_path=b), self.remote_options["remote_exec_path"] becomes a list, and os.path.dirname(str([...])) produces nonsense like ''. Either reject duplicates (raise ValueError), or always take v[-1]/v[0].
7. Fixed remote engine filename → no concurrency safety.
self.remote_engine_path = "trtexec_benchmark_model.trt" is hardcoded and lands in the remote user's home dir. Two autotune runs against the same target will clobber each other's engines mid-benchmark. Suggest deriving from a tempfile-style suffix (PID + uuid) or the local self.engine_path basename.
8. --useCudaGraph is silently injected into the remote command.
The remote trtexec_safe and trtexec --safe invocations always append --useCudaGraph, regardless of what the user passed in trtexec_args. This was added in commit 110f3d7f ("more stable when using cudaGraphs"). Worth either documenting the override on --remoteAutoTuningConfig or making it conditional on user opt-in — silently flipping a kernel-launch mode for latency measurement is surprising.
Suggestions
9. Module-level regex constants should be private.
safe_pattern/std_pattern are exported by virtue of being module-level. They're internal implementation. _SAFE_LATENCY_PATTERN / _STD_LATENCY_PATTERN would scope and signal intent.
10. Stale comment on warning text.
The except ImportError warning was trimmed in commit 2512076 after CodeRabbit flagged it — please double-check the final string still makes sense in isolation; it now reads as a bare statement rather than an actionable warning.
11. Document that sshpass is now an implicit runtime dependency.
If --remoteAutoTuningConfig includes a password, sshpass must be installed on the host running ModelOpt. That's not in any docstring or README; users will discover it via the misleading "trtexec binary not found" message above.
12. Consider redacting credentials from remote-config error messages.
The ValueError raises that include arg or config_arg_value substrings can echo a ssh://user:pass@host back at the user. Most are caught early enough that the password isn't yet parsed, but f"Malformed --remoteAutoTuningConfig argument: {arg}" could echo a password. Strip the password before formatting.
Positive
- Good iteration on review feedback: query-param validation,
shlex.quoteon every interpolated remote-side token, explicitValueErroron missing user/host,urlparse-based URL parsing rather than regex. - Switching from the rolled-up
[I] Latency:line to[I] GPU Compute Time:is the right call —Latency:includes H2D/D2H copies, which is noisy for autotune comparisons. ImportErroris now re-raised (was previously swallowed and silently stripped from args), so a TRT < 10.15 environment fails fast.- Hard fail on multiple
--remoteAutoTuningConfigargs closes off an ambiguous configuration.
Test Coverage Assessment
- Unit-test coverage for new code: 0.
- Existing tests: 2 mocked tests will regress (see Critical #1).
- No GPU-CI run posted on the PR (vetting still pending).
- No regression test for the original bug — a mock that returns the
[I] GPU Compute Time:format (or simulates the remote SSH path) would lock the fix in place.
Final Verdict
Request Changes.
Blocking items: the test regression (Critical #1), the missing tests for new functionality (Critical #2), and the unresolved # nosec situation (Important #3). The remote-execution logic itself is reasonable, and the iteration history shows the author has been responsive to security feedback — but the PR can't land in its current state because it breaks pre-existing tests.
|
I've addressed all comments. The following remain unresolved: |
ajrasane
left a comment
ajrasane
left a comment
There was a problem hiding this comment.
Review
Fixes a real bug (safety-runtime + remoteAutoTuning returning inf latencies) and the refactor into testable helpers (_parse_remote_autotuning_url, _RemoteAutotuningConfig, _redact_url_password, etc.) is clean. New unit tests in test_trtexec_benchmark.py (728 lines) and test_cli_pipeline.py (211 lines) close the prior "no tests for new logic" gap. A few items below are blocking; the rest are quick-fix follow-ups.
Critical
-
All three remote
subprocess.runcalls have notimeout=—modelopt/onnx/quantization/autotune/benchmark.pylines 430 (scp), 453 (trtexec_safe ssh), and 471 (fallback ssh). A hung SSH (network blip, host-key prompt becauseStrictHostKeyCheckingisn't set, keyboard-interactive auth fallback) will block the entire autotuning workflow forever. CodeRabbit flagged this and marked it resolved against commit 28e4e8e, but it is not in HEAD — please re-addtimeout=on all three calls and asubprocess.TimeoutExpiredhandler that logs and returnsfloat("inf"). -
remote_user/remote_ipnot validated against leading-(argv smuggling). The scp destination is built asf"{self.remote_user}@{self.remote_ip}:..."andremote_user/remote_ipcome fromurlparse.urlparsedoes not reject usernames or hostnames that look like options. A URL likessh://-oProxyCommand=evil@host/...lets a crafted username be reinterpreted as anssh/scpflag (CVE-2017-1000117 class). Please rejectremote_user/remote_ipthat start with-in_parse_remote_autotuning_url.
Important
-
Fallback path uses
_STD_PATTERNbut rantrtexec --safe(line 475). The--safeinvocation oftrtexectypically emits theAverage over N runs - GPU latency:line (i.e.,_SAFE_PATTERN), notGPU Compute Time: ... median = .... If the safety-proxy target emits the SAFE format, parsing silently returnsinf. Either try both patterns in the fallback, or switch the fallback to_SAFE_PATTERN. -
stderris dropped in the consolidated failure log (lines 478–480):f\"Failed to run trtexec_safe or trtexec with '--safe'\n {result.stdout}\". SSH/sshpass auth failures, host-key prompts, and connection-refused errors are emitted on stderr. Includeresult.stderrso users can diagnose connection failures. -
--useCudaGraphis silently injected into the remote command (lines 445, 467) regardless of the user'strtexec_args. Document this behavior, and consider gating it (only add if not already present). -
No cleanup of the remote engine file.
remote_engine_path = \"trtexec_benchmark_model.trt\"is scp'd every run and never removed; repeated sessions accumulate stale files on the target. Add anssh ... rm -f <shlex.quote(remote_engine_path)>in afinally. -
Hardcoded
remote_engine_path. Concurrency is disallowed by design, but unrelated users on the same target box can still clash. Suggestf\"trtexec_benchmark_model_{os.getpid()}_{uuid.uuid4().hex}.trt\". -
self.remote_optionsis populated but never read inrun(). Either remove it or document why it's kept on the instance. -
Backward compatibility of
_STD_PATTERN. It now matches[I] GPU Compute Time: ... median = X msinstead of[I] Latency: ... median = X ms. The remote path is gated at>=10.15, but the local std path has no version guard — oldertrtexecmay still emitLatency:. Either try both patterns or pin the minimum TensorRT version.
Suggestions
- Add
-oStrictHostKeyChecking=accept-newto thessh/scpinvocations so first-connect doesn't hang (becomes critical without timeouts). _ensure_remote_autotuning_flagsappends--safeif missing, which meansself.is_safeends up coupled to "remote autotuning is configured". Worth a docstring note.- Docstring typo in
docs/source/guides/9_autotune.rst: "Only once instance" → "Only one instance". ["-P", str(self.remote_port)]would be more consistent with thesshinvocation right below it thanf\"-P{self.remote_port}\".
CI
unit-pr-required-checkandcheck-dco / waitare failing on HEAD — must be green before merge.
Verdict
Request Changes. Primary blockers: missing timeout= on all three remote subprocess.run calls, and argv-smuggling validation on remote_user/remote_ip. The fallback latency-pattern mismatch is also worth fixing in the same pass.
… push engine to the remote device for correct latency testing Signed-off-by: dmoodie <dmoodie@nvidia.com>
fff917e to
745bc49
Compare
Signed-off-by: dmoodie <dmoodie@nvidia.com>
Signed-off-by: dmoodie <dmoodie@nvidia.com>
Signed-off-by: dmoodie <dmoodie@nvidia.com>
Signed-off-by: dmoodie <dmoodie@nvidia.com>
…Model-Optimizer into dmoodie/bugfix/trtexec_safe
…t on completion Signed-off-by: dmoodie <dmoodie@nvidia.com>
Signed-off-by: dmoodie <dmoodie@nvidia.com>
… build Signed-off-by: dmoodie <dmoodie@nvidia.com>
|
All three remote subprocess.run calls have no timeout=
remote_user / remote_ip not validated against leading - (argv smuggling).
Fallback path uses _STD_PATTERN but ran trtexec --safe
stderr is dropped in the consolidated failure log
--useCudaGraph is silently injected into the remote command
No cleanup of the remote engine file.
Hardcoded remote_engine_path
self.remote_options is populated but never read
Backward compatibility of _STD_PATTERN
Docstring typo ...
Add -oStrictHostKeyChecking=accept-new
|
…w wasn't available before, this is NOT a breaking change Signed-off-by: dmoodie <dmoodie@nvidia.com>
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # trtexec_safe / trtexec --safe emit "GPU Compute Time"; local trtexec emits "Latency" | ||
| _latency_pattern = ( | ||
| r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms" | ||
| if self._remote_use_trtexec_safe | ||
| else r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" | ||
| ) |
There was a problem hiding this comment.
[CRITICAL Algorithm] The pattern is selected from _remote_use_trtexec_safe, but it must be selected from has_remote_config — the fallback branch parses the wrong line and re-introduces the bug this PR fixes.
What's wrong: the comment on line 621 states the invariant correctly — "trtexec_safe / trtexec --safe emit GPU Compute Time; local trtexec emits Latency". The condition contradicts it. When the probe finds no trtexec_safe, _remote_use_trtexec_safe is False, the remote command becomes trtexec --safe … (line 571-572), and this expression then picks \[I\]\s+Latency: and applies it to that remote output.
Why it matters: on any remote host without a trtexec_safe binary — i.e. the entire documented fallback path, and the one the CHANGELOG advertises ("falling back to trtexec --safe otherwise") — the regex never matches, so run() returns inf for every candidate. Autotune then sees inf everywhere and degenerates to the exact failure mode in the PR description (Baseline latency: inf ms). The binary_name expression at line 611-615 keys off has_remote_config for the same distinction, which shows the intent.
It also makes two of the new unit tests red — test_probe_absent_uses_trtexec_safe_flag (test_trtexec_benchmark.py:757) and test_remote_run_uses_safe_flag_when_probe_absent (:835) both set/produce _remote_use_trtexec_safe = False, feed [I] GPU Compute Time: median = …, and assert a parsed latency (3.5 and 5.55). With this condition both get inf. I could not execute pytest in this environment, so that part is from reading the code, but the regex has no Latency: substring to match against those mocks.
Fix:
| # trtexec_safe / trtexec --safe emit "GPU Compute Time"; local trtexec emits "Latency" | |
| _latency_pattern = ( | |
| r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms" | |
| if self._remote_use_trtexec_safe | |
| else r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" | |
| ) | |
| # trtexec_safe / trtexec --safe emit "GPU Compute Time"; local trtexec emits "Latency" | |
| _latency_pattern = ( | |
| r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms" | |
| if self.has_remote_config | |
| else r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" | |
| ) |
| mock_result = MagicMock() | ||
| mock_result.returncode = 0 | ||
| mock_result.stdout = "[I] Latency: min = 2.50 ms, max = 4.00 ms, median = 3.14 ms" | ||
| mock_result.stdout = "[I] GPU Compute Time: min = 2.50 ms, max = 4.00 ms, median = 3.14 ms" |
There was a problem hiding this comment.
[CRITICAL Algorithm] This mock (and the identical one at line 214) now feeds a string the local code path cannot parse, so both tests fail.
The trtexec_bench fixture (line 61-66) is constructed with no trtexec_args, so has_remote_config is False and _remote_use_trtexec_safe stays None. run() therefore selects the local pattern r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" (benchmark.py:625), which has nothing to match in "[I] GPU Compute Time: …" → re.search returns None → run() returns inf, and the pytest.approx(3.14) / pytest.approx(5.0) assertions fail.
An earlier review round flagged these two mocks as stale against a version of the code that used GPU Compute Time unconditionally; the current code correctly keeps Latency for the local trtexec path, so the mocks should go back to the original strings. Both of these are local-benchmark tests — remote coverage now lives in tests/unit/onnx/quantization/autotune/test_trtexec_benchmark.py.
| mock_result.stdout = "[I] GPU Compute Time: min = 2.50 ms, max = 4.00 ms, median = 3.14 ms" | |
| mock_result.stdout = "[I] Latency: min = 2.50 ms, max = 4.00 ms, median = 3.14 ms" |
| mock_result = MagicMock() | ||
| mock_result.returncode = 0 | ||
| mock_result.stdout = "[I] Latency: min = 4.00 ms, max = 6.00 ms, median = 5.00 ms" | ||
| mock_result.stdout = "[I] GPU Compute Time: min = 4.00 ms, max = 6.00 ms, median = 5.00 ms" |
There was a problem hiding this comment.
[CRITICAL Algorithm] Same as line 196 — trtexec_bench is a local (non-remote) benchmark, so run() matches [I] Latency:, this mock yields inf, and pytest.approx(5.0) fails.
| mock_result.stdout = "[I] GPU Compute Time: min = 4.00 ms, max = 6.00 ms, median = 5.00 ms" | |
| mock_result.stdout = "[I] Latency: min = 4.00 ms, max = 6.00 ms, median = 5.00 ms" |
| scp_cmd = [ | ||
| "scp", | ||
| "-P", | ||
| str(self.remote_port), | ||
| "-oStrictHostKeyChecking=accept-new", | ||
| self.engine_path, | ||
| f"{self.remote_user}@{self.remote_ip}:{shlex.quote(self.remote_engine_path)}", | ||
| ] |
There was a problem hiding this comment.
[IMPORTANT Performance] Neither scp/ssh invocation sets -oBatchMode=yes, so an auth failure blocks for the full network_timeout_seconds (default 10 minutes) — per candidate.
The docs added in this PR now require key-based auth, and _parse_remote_autotuning_url deliberately drops parsed.password. So a user who passes ssh://user:pass@host (the form the old code supported, and the form _redact_url_password still exists to scrub) gets no key auth configured. subprocess.run inherits the parent's stdin, so ssh falls through to an interactive password prompt and hangs until the timeout expires. With one autotune run evaluating dozens of candidates that is hours of wall-clock producing nothing but inf.
Adding -oBatchMode=yes to all three commands (scp at line 540, the run at line 573, the probe at _probe_remote_trtexec_safe) makes a misconfigured host fail in seconds with a clear stderr instead of stalling, which matches this PR's stated goal of failing fast on bad configuration:
scp_cmd = [
"scp",
"-P",
str(self.remote_port),
"-oStrictHostKeyChecking=accept-new",
"-oBatchMode=yes",
self.engine_path,
f"{self.remote_user}@{self.remote_ip}:{shlex.quote(self.remote_engine_path)}",
]
| * TensorRT 10.15 or later — passing ``--remoteAutoTuningConfig`` on an older build raises ``ImportError`` immediately rather than falling back to local benchmarking. | ||
| * Valid remote autotuning configuration — a malformed or missing ``--remoteAutoTuningConfig`` URL raises ``ValueError`` with a descriptive message. |
There was a problem hiding this comment.
[IMPORTANT Compatibility] These two documented contracts don't hold for any of the public entry points — the exceptions are swallowed one frame up.
TrtExecBenchmark.__init__ does raise ImportError / ValueError now, but every caller goes through init_benchmark_instance (autotune/workflows.py), which wraps construction in a blanket handler:
except Exception as e:
logger.error(f"TensorRT initialization failed: {e}", exc_info=True)
return NoneSo what users actually observe is:
modelopt.onnx.quantization.quantize(...)→RuntimeError("Failed to initialize TensorRT benchmark")(quantize.py:372), notValueError/ImportError.python -m modelopt.onnx.quantization.autotune→logger.error("Failed to initialize TensorRT benchmark")and exit code 1 (autotune/__main__.py:116-118).
The descriptive message does reach the log via exc_info=True, so it isn't invisible — but "raises ImportError immediately" / "raises ValueError with a descriptive message" is not the API behaviour, and a caller writing except ValueError: around quantize() to detect a bad URL will never catch it. This is also the mechanism behind the PR description's "no longer gracefully fallback but rather fail hard and fast": the hard failure is downgraded to a generic RuntimeError/exit-1 before it reaches the user's except.
Pick one and make doc and code agree — either let these two exception types escape init_benchmark_instance (narrow the except Exception so ValueError/ImportError propagate), or reword these bullets to describe the RuntimeError / exit-1 behaviour. If you go the propagate route, note that none of the new tests cover it — test_cli_pipeline.py mocks init_benchmark_instance out entirely — so a test asserting pytest.raises(ValueError) through init_benchmark_instance would be worth adding.
| except Exception as e: | ||
| self.logger.warning( | ||
| f"Remote binary probe failed ({e}); " | ||
| "will use 'trtexec --safe' for all candidates in this run" | ||
| ) | ||
| return False |
There was a problem hiding this comment.
[SUGGESTION] The probe conflates "trtexec_safe is genuinely absent" with "the probe could not run", and caches the latter for the whole autotune run.
Any ssh transport failure — host unreachable, key rejected, ssh not on PATH (FileNotFoundError), timeout — lands in this except Exception and returns False, which is then cached in _remote_use_trtexec_safe and never re-evaluated. The run continues, every candidate goes down the trtexec --safe branch, and each one fails at the scp/ssh step and returns inf. The user gets one WARNING at the top and then a wall of inf latencies with no indication the host was never reachable.
Since the probe's transport is identical to the one the benchmark itself needs, a transport failure here reliably predicts that the whole run is doomed — distinguishing the two cases would let it fail fast per this PR's stated intent:
try:
result = subprocess.run(
probe_cmd, capture_output=True, text=True, timeout=self.network_timeout_seconds
) # nosec B603 — list-form, no shell=True; user/host validated against leading -
except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e:
raise RuntimeError(f"Cannot reach remote host for autotuning: {e}") from e
if result.returncode == 0:
self.logger.debug(f"trtexec_safe found at {safe_binary!r} on remote host")
return True
self.logger.warning(
f"trtexec_safe not found at {safe_binary!r} on remote host "
f"(probe exit {result.returncode}); "
"will use 'trtexec --safe' for all candidates in this run"
)
return FalseRelated, on the same method: if remote_exec_path has no directory component, remote_bin_path is "", so safe_binary is the bare relative name trtexec_safe and test -x trtexec_safe resolves against the remote home directory rather than PATH — it always reports absent. The fallback trt_path (line 571) becomes bare trtexec, which does resolve via PATH inside the ssh command, so the two paths disagree for that input. Either reject a remote_exec_path without a directory in _parse_remote_autotuning_url, or probe with command -v when remote_bin_path is empty.
| self.logger.error(f"stderr: {_redact_url_password(result.stderr)}") | ||
| return float("inf") | ||
| if self.has_remote_config: | ||
| # need to push the model to the device and use trtexec_safe to run |
There was a problem hiding this comment.
[SUGGESTION] Stale comment: what gets pushed is the built engine (self.engine_path, line 545), not the model. The local trtexec call above already consumed the ONNX and wrote the engine under --skipInference. Suggest # push the built engine to the device and measure it with trtexec_safe.
There was a problem hiding this comment.
Claude review — 1 blocking correctness bug (with a red test suite behind it)
Scope: trigger comment was a bare /claude review, so full scope. 11 files changed (+2025/-55). Reviewed all 5 modelopt/ files, docs/source/guides/9_autotune.rst, CHANGELOG.rst, and the four test files. I could not execute pytest in this environment, so the test-failure claims below are from reading the code paths, not from a run.
Findings: CRITICAL 3, IMPORTANT 2, SUGGESTION 2
The one that matters
benchmark.py:621-626 selects the latency regex from self._remote_use_trtexec_safe instead of self.has_remote_config. The comment directly above it states the correct invariant — trtexec_safe / trtexec --safe emit GPU Compute Time; local trtexec emits Latency — and the condition contradicts it. When the probe finds no trtexec_safe, the remote command becomes trtexec --safe … but the parser looks for [I] Latency:, never matches, and run() returns inf for every candidate.
That is the entire documented fallback path — the one the CHANGELOG advertises as falling back to trtexec --safe otherwise — reproducing the exact Baseline latency: inf ms failure this PR sets out to fix. The binary_name expression 10 lines up (:611-615) already keys off has_remote_config for the same distinction, so the intent is clear; it is a one-word fix.
It also leaves four tests red, in both directions:
tests/unit/.../test_trtexec_benchmark.py:757and:835set_remote_use_trtexec_safe = False, feed[I] GPU Compute Time: median = …, and assert 3.5 / 5.55 — they getinf.tests/gpu/.../test_benchmark.py:196and:214had their mocks switched toGPU Compute Time, but thetrtexec_benchfixture has no remote config, so those go through the localLatencypattern — they also getinf. Those two mocks should go back to[I] Latency:; a prior review round flagged them against an earlier revision that usedGPU Compute Timeunconditionally, and the source has since (correctly) keptLatencyfor the local path.
Also worth fixing before merge
- Docs promise exceptions the code swallows (
9_autotune.rst:253-254).TrtExecBenchmark.__init__raisesImportError/ValueErroras documented, but every caller routes throughinit_benchmark_instance, whose blanketexcept Exception: return Noneconverts them toRuntimeError('Failed to initialize TensorRT benchmark')forquantize()and to exit-1 for the CLI. The descriptive text still reaches the log viaexc_info=True, so it is not invisible — but the PR's headline fail hard and fast so the user may correct the configuration does not survive that frame, andexcept ValueErroraroundquantize()will never fire. Nothing in the new tests covers this path (test_cli_pipeline.pymocksinit_benchmark_instanceout entirely). - No
-oBatchMode=yeson any scp/ssh call._parse_remote_autotuning_urlnow dropsparsed.password, and the docs require key auth — so assh://user:pass@hostURL (still the reason_redact_url_passwordexists) gets no usable credential,sshinherits stdin and prompts interactively, and each candidate stalls for the fullnetwork_timeout_seconds(default 10 min) before returninginf.
Suggestions (non-blocking)
_probe_remote_trtexec_safereturnsFalsefor transport failures (unreachable host, rejected key, missingssh) just as it does for a genuinely absent binary, and caches that for the whole run. Also, aremote_exec_pathwith no directory component makestest -x trtexec_saferesolve against the remote home dir instead ofPATH, so the probe and the fallbacktrt_pathdisagree for that input.- Stale comment at
:539— the engine is pushed, not the model.
Assessment
Risk: medium-high. The structural work here is good and clearly responsive to earlier rounds — urlparse-based validation, duplicate-param rejection, the argv-smuggling guard on user/host, shlex.quote on every remote-side token, per-instance UUID engine names, credential redaction in logs, once-per-run binary probing so candidates stay comparable, and finally-scoped remote cleanup are all the right calls. The single inverted predicate is what blocks it: it silently zeroes out the fallback path and takes four tests with it. Everything else is small.
🤖 Generated with Claude Code
Signed-off-by: dmoodie <dmoodie@nvidia.com>
…delopt into dmoodie/bugfix/trtexec_safe Signed-off-by: dmoodie <dmoodie@nvidia.com>
…ofiling Signed-off-by: dmoodie <dmoodie@nvidia.com>
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Prior concerns are resolved and the remote path now reads correctly, but two open questions remain before this can merge.
Needs action:
- Narrow the new
except (ValueError, ImportError): raiseinautotune/workflows.py:141— it also escapes forTensorRTPyBenchmark's "TensorRT not available" / "PyTorch CUDA not available"ImportError, so a plain missing-TRT install now produces a raw traceback out ofrun_autotune()/quantize()instead of the previous "Failed to initialize TensorRT benchmark" + exit 1. Confirm intent or scope the re-raise to the remote-config path, and note it inCHANGELOG.rst. - 💬 Security Exception section is now in the PR body and each site has a why-safe comment — still needs an explicit review request to
@NVIDIA/modelopt-setup-codeownersfor the four# nosec B603and the# nosec B404perSECURITY.mdstep 2. - Use
self.has_remote_configinstead ofself._remote_use_trtexec_safe is not Nonefor latency-pattern selection inbenchmark.py:622— equivalent today but fragile. - Add
--warmUp/--iterationsto the forwarded-flags bullet indocs/source/guides/9_autotune.rst:259.
No action needed:
- ✔️ 5 previous concerns addressed, including the
FileNotFoundErrorfilename and movingscpinside thetry/finally.
|
/claude review |
|
/ok to test 65cd70d |
| remote_run_cmd = [ | ||
| "ssh", | ||
| "-oBatchMode=yes", | ||
| "-oStrictHostKeyChecking=accept-new", | ||
| "-p", | ||
| f"{self.remote_port}", | ||
| f"{self.remote_user}@{self.remote_ip}", | ||
| f"{ld_path} {shlex.quote(trt_path)} {extra_flags}--useCudaGraph " | ||
| f"--warmUp={self.warmup_runs} --iterations={self.timing_runs} " | ||
| f"--avgRuns={self.timing_runs} --duration=0 " | ||
| f"--loadEngine={shlex.quote(self.remote_engine_path)}", | ||
| ] |
There was a problem hiding this comment.
[IMPORTANT Algorithm] User-supplied measurement flags from --trtexec_benchmark_args are not forwarded to the remote run, so for dynamic-shape engines the measurement happens at a shape the user never asked for.
self.trtexec_args is folded into _base_cmd (line 511) and therefore reaches the local build, but remote_run_cmd here is fully hardcoded: --useCudaGraph --warmUp --iterations --avgRuns --duration=0 --loadEngine. Every inference-affecting flag the user passed is silently dropped at measurement time — most consequentially --shapes / --minShapes / --optShapes / --maxShapes, but also --useSpinWait, --infStreams, --streams, --noDataTransfers.
Why it matters: a user who builds with --minShapes=... --optShapes=... --maxShapes=... gets an engine with a multi-shape profile, and the remote --loadEngine call selects a profile shape on its own rather than the one the user intended to tune for. Every candidate is then ranked on a shape nobody requested, which is exactly the kind of silently-wrong ranking autotune cannot recover from — unlike the pre-fix inf, this failure is invisible.
The docs added in this PR compound it — docs/source/guides/9_autotune.rst:76 states:
--useCudaGraph,--avgRuns, and--duration=0are forwarded to the remotetrtexec_safe(ortrtexec --safe) call so measurement settings match those used during local engine builds.
Neither half holds: these three are hardcoded rather than forwarded, and --useCudaGraph / --duration=0 never appear in the local build command at all.
Suggested fix — forward a whitelist of measurement-relevant user args into the remote command, e.g.:
_REMOTE_FORWARDED_PREFIXES = (
"--shapes", "--minShapes", "--optShapes", "--maxShapes",
"--useSpinWait", "--infStreams", "--streams", "--noDataTransfers",
)
forwarded = [
shlex.quote(a)
for a in self.trtexec_args
if a.startswith(_REMOTE_FORWARDED_PREFIXES)
]
remote_run_cmd = [
"ssh", "-oBatchMode=yes", "-oStrictHostKeyChecking=accept-new",
"-p", f"{self.remote_port}",
f"{self.remote_user}@{self.remote_ip}",
" ".join([
ld_path, shlex.quote(trt_path), *( [extra_flags.strip()] if extra_flags else [] ),
"--useCudaGraph",
f"--warmUp={self.warmup_runs}",
f"--iterations={self.timing_runs}",
f"--avgRuns={self.timing_runs}",
"--duration=0",
*forwarded,
f"--loadEngine={shlex.quote(self.remote_engine_path)}",
]),
]At minimum, if forwarding is deliberately out of scope for this PR, please correct the docs sentence to say the remote measurement flags are fixed and that shape/stream flags in --trtexec_benchmark_args apply to the build only.
There was a problem hiding this comment.
Dynamic shapes are not permitted in safety thus not used in remote benchmarking. We should not need to reproduce the safety restrictions here as we want to maintain one source of truth. Thus assume the user is already familiar with the external restrictions.
| user=parsed.username, | ||
| ip=parsed.hostname, | ||
| port=port, | ||
| bin_path=os.path.dirname(options["remote_exec_path"]), |
There was a problem hiding this comment.
[SUGGESTION] Deriving the remote binary directory by discarding remote_exec_path's basename makes two inputs fail quietly.
- No directory component.
remote_exec_path=trtexec(relying on the remotePATH) yieldsbin_path == "", soos.path.join("", "trtexec_safe")is the bare relative string"trtexec_safe". The probe then runstest -x trtexec_safe, which resolves against the SSH login cwd rather thanPATH, returns non-zero, and the run falls back totrt_path = "trtexec"(line 573) — also relative, also likely not found. Every candidate returnsinfwith only a "not found at ... on remote host" warning to go on. - Non-default basename.
remote_exec_path=/opt/trt/bin/trtexec_reldiscardstrtexec_reland reconstructs/opt/trt/bin/trtexecfor the fallback path, which need not exist even though the user gave a working binary.
Both become loud and correct with one validation plus reusing the user's own path for the fallback:
exec_path = options["remote_exec_path"]
if not os.path.isabs(exec_path):
raise ValueError(
"remote_exec_path in --remoteAutoTuningConfig must be an absolute path "
f"on the remote device (got {exec_path!r})"
)
return _RemoteAutotuningConfig(
...
exec_path=exec_path, # used as the `trtexec --safe` fallback binary
bin_path=os.path.dirname(exec_path), # used to locate `trtexec_safe`
...
)tests/unit/.../test_trtexec_benchmark.py:170 already asserts bin_path == "/opt/trt/bin" for an absolute input, so an absolute-path requirement costs nothing in coverage.
| # trtexec_safe / trtexec --safe emit "GPU Compute Time"; local trtexec emits "Latency" | ||
| _latency_pattern = ( | ||
| r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms" | ||
| if self._remote_use_trtexec_safe is not None | ||
| else r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" | ||
| ) |
There was a problem hiding this comment.
[SUGGESTION] The regex selector keys off the wrong state variable. It happens to be correct today, but only by an invariant that isn't stated anywhere.
self._remote_use_trtexec_safe is not None is a proxy for "the probe has run", which is a proxy for "we are in remote mode". It works only because the probe at line 567-568 is unconditionally reached whenever has_remote_config is true and we get this far. Any future early-return or reordering between the probe and this line silently flips a remote run onto the local Latency pattern, which yields inf for every candidate — the exact failure mode this PR exists to fix, and the one a prior review round already caught in an earlier form of this expression.
The condition the comment above actually describes is has_remote_config, which is also what binary_name uses 10 lines up (line 617):
| # trtexec_safe / trtexec --safe emit "GPU Compute Time"; local trtexec emits "Latency" | |
| _latency_pattern = ( | |
| r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms" | |
| if self._remote_use_trtexec_safe is not None | |
| else r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" | |
| ) | |
| # trtexec_safe / trtexec --safe emit "GPU Compute Time"; local trtexec emits "Latency" | |
| _latency_pattern = ( | |
| r"\[I\]\s+GPU Compute Time:.*?median\s*=\s*([\d.]+)\s*ms" | |
| if self.has_remote_config | |
| else r"\[I\]\s+Latency:.*?median\s*=\s*([\d.]+)\s*ms" | |
| ) |
Note tests/unit/.../test_trtexec_benchmark.py:757 and :835 set _remote_use_trtexec_safe directly on a remote_bench fixture, so they pass under either predicate.
There was a problem hiding this comment.
Somehow the agents are still wrong. We must key off of latency when using the trtexec binary which can still be remote with --safe and we need to key off of GPU Compute Time when using the trtexec_safe binary.
| network_timeout_minutes=args.network_timeout_minutes, | ||
| remote_engine_path=args.remote_engine_path, |
There was a problem hiding this comment.
[SUGGESTION] Now that init_benchmark_instance re-raises ValueError/ImportError instead of returning None, this call site loses the CLI's documented exit-code contract.
The init_benchmark_instance(...) call sits above the try: at line 120, so a malformed --remoteAutoTuningConfig, a plugin_libraries + remote clash, or TensorRT < 10.15 now escapes run_autotune as an uncaught exception. run_autotune's own docstring (lines 89-93) promises "1: Autotuning failed (exception occurred)", and the if benchmark_instance is None: return 1 branch at line 116 becomes dead for exactly the config errors this PR is about. The user gets a Python traceback rather than the descriptive message.
Failing fast is the right call per the PR description — this is just about presenting it cleanly:
try:
benchmark_instance = init_benchmark_instance(
use_trtexec=args.use_trtexec,
plugin_libraries=args.plugin_libraries,
timing_cache_file=args.timing_cache,
warmup_runs=args.warmup_runs,
timing_runs=args.timing_runs,
trtexec_args=trtexec_args,
network_timeout_minutes=args.network_timeout_minutes,
remote_engine_path=args.remote_engine_path,
)
except (ValueError, ImportError) as e:
logger.error(f"Invalid benchmark configuration: {e}")
return 1
if benchmark_instance is None:
logger.error("Failed to initialize TensorRT benchmark")
return 1test_cli_pipeline.py mocks init_benchmark_instance out entirely, so neither the raise nor the exit code is covered today.
There was a problem hiding this comment.
Claude review — CRITICAL 0, IMPORTANT 1, SUGGESTION 3
Scope: bare /claude review, so full scope. 10 files changed (+2030/-53). Reviewed all 5 modelopt/onnx/ source files, docs/source/guides/9_autotune.rst, CHANGELOG.rst, and targeted reads of the new tests. Note that git diff origin/main HEAD here also surfaces reverse-diffs of modelopt/onnx/autocast/, modelopt/onnx/export/, and the graph_utils split because main has advanced past this branch; those were excluded using the PR's authoritative file list and are not reviewed below.
Prior rounds are well addressed. Every blocking item from the two earlier reviews is resolved in this revision: the inverted _remote_use_trtexec_safe predicate that killed the trtexec --safe fallback, -oBatchMode=yes on all three ssh/scp invocations, plugin_libraries + remote now raising in __init__ with a matching docs note, init_benchmark_instance re-raising ValueError/ImportError so config errors are not flattened into RuntimeError, the changelog entries moved under Backward Breaking Changes, and the stale gpu-test mocks reverted. The defensive work is genuinely solid — urlparse-based validation with duplicate-key and port-range rejection, the leading-- argv-smuggling guard on user and host, shlex.quote on every remote-side token, _redact_url_password across all logged output, per-run UUID engine filenames, finally-scoped remote cleanup, and probing the remote binary once so all candidates stay comparable.
Most impactful finding
User measurement flags are not forwarded to the remote run (IMPORTANT, benchmark.py:575-586). self.trtexec_args reaches the local build via _base_cmd, but remote_run_cmd is fully hardcoded, so --shapes / --minShapes / --optShapes / --maxShapes, --useSpinWait, --infStreams, --streams, and --noDataTransfers are all dropped at measurement time. For a dynamic-shape engine that means every candidate is ranked at a profile shape trtexec picks on its own rather than the one the user tuned for — and unlike the pre-fix inf, that failure is silent. docs/source/guides/9_autotune.rst:76 compounds it by asserting these flags are forwarded "so measurement settings match those used during local engine builds"; in fact they are hardcoded, and --useCudaGraph / --duration=0 never appear in the local build at all. Either forward a whitelist (patch in the inline comment) or correct the docs to say the remote measurement flags are fixed.
Suggestions (non-blocking)
benchmark.py:306—bin_path = dirname(remote_exec_path)fails quietly on two inputs: a PATH-relativeremote_exec_path=trtexecmakes both the probe and the fallback resolve against the SSH login cwd instead ofPATH, and a non-default basename liketrtexec_relis discarded in favour of a reconstructeddirname/trtexecthat need not exist. Requiring an absolute path and reusing it as the fallback binary makes both loud.benchmark.py:625-630— the regex selector keys off_remote_use_trtexec_safe is not None(a proxy for "the probe ran") where the comment directly above describeshas_remote_config, whichbinary_namealready uses 10 lines up. Correct today only by an unstated invariant; any future early-return between the probe and this line puts a remote run back on theLatencypattern and returnsinffor everything.autotune/__main__.py:105-118— theinit_benchmark_instancecall sits above thetry:, so the newly-raised config errors escaperun_autotuneas a traceback, breaking its documented "1: Autotuning failed" contract and making thebenchmark_instance is Nonebranch dead for those errors. Failing fast is right; catching and logging keeps it clean.- Still open from a prior round:
log_filerecords only the build output (benchmark.py:517), never the remote timing output the latency regex actually parses — so aninfcandidate is not diagnosable from the saved artifact, which is the main reason the log exists.
Risk assessment
Low-moderate, well-contained. Everything sits behind --use_trtexec in the ONNX autotune path; no modelopt_state schema, mode registration, export path, or public modelopt/torch API is touched, so there is no checkpoint or restore-fidelity exposure. The GPU Compute Time metric switch is now correctly scoped to remote mode, leaving purely local --use_trtexec users on Latency as before. Residual risk is concentrated in the one IMPORTANT item, which affects dynamic-shape models specifically and is a silent mis-ranking rather than a hard failure.
🤖 Generated with Claude Code
|
please rebase your PR so merge conflicts is fixed and also see if any claude review comment is pending to be addressed or not |
Signed-off-by: dmoodie <dmoodie@nvidia.com>
…izer into dmoodie/bugfix/trtexec_safe Signed-off-by: dmoodie <dmoodie@nvidia.com>
|
/claude review |
|
@claude review |
Claude review in progress
|
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6.1-sol) — DM the bot to share feedback.
Changes requested: 548 added logic lines require splitting, and remote-path handling and malformed-config logging still need fixes.
Needs action:
- ✂️ Split into
[1/3]autotune/benchmark.pyhelpers,[2/3]remote execution plusautotune/workflows.py/__main__.py,[3/3]parentquantization/wiring. The 520-line autotune slice needs this further cut. Link siblings; state linear merge order; each builds, includes tests, and passes CI independently. - Redact malformed remote-config arguments in
benchmark.py; add a credential-leak regression test (inline). - Preserve the supplied
remote_exec_pathfor fallback execution inbenchmark.py; test a nonstandard binary basename (inline). - 💬 Author requested a security exception — still request explicit
@NVIDIA/modelopt-setup-codeownersapproval for the five new Bandit suppressions, as required bySECURITY.md. - Explain in the PR body why extending
TrtExecBenchmarkwith stdlib SSH subprocesses is preferable to reusingtools/launcher/core.pyor adding Paramiko/Fabric; incorporate the QNX/manual-execution rationale from the replies.
No action needed:
- ✔️ Prior timeout, key-authentication, UUID-default, cleanup, plugin-rejection, CLI-error-handling, and remote-log concerns are resolved. New test headers match
LICENSE_HEADER; existing integration coverage is extended, not weakened. Tests were inspected, not run.
| ip: str | ||
| port: int | ||
| bin_path: str # dirname of ``remote_exec_path`` — directory containing trtexec_safe / trtexec | ||
| lib_path: str # value of ``remote_lib_path`` |
There was a problem hiding this comment.
Bot comment.
The malformed-argument branch logs and raises with raw arg. For example, --remoteAutoTuningConfigssh://alice:secret@host?... reaches this branch and exposes the password through both the debug log and the CLI's logger.error(str(e)). The normal command/output paths are now redacted, but this validation path still bypasses that protection. Redact arg before formatting either message (or omit the supplied value), and add a regression test checking that neither logs nor the exception contain the password.
| # Flags from trtexec_args forwarded to the remote measurement run (--loadEngine=). | ||
| # --minShapes / --optShapes / --maxShapes are build-time only: they define optimization profiles | ||
| # embedded in the engine and are meaningless (or error-prone) with --loadEngine. They already | ||
| # reach the local engine build via _base_cmd.extend(trtexec_args); do not repeat them here. |
There was a problem hiding this comment.
Bot comment.
Requiring an absolute remote_exec_path fixes the relative-path case, but the parser still discards the supplied basename. A valid /opt/trt/bin/trtexec_rel config builds using that configured executable, then the fallback reconstructs /opt/trt/bin/trtexec, which may not exist. Preserve an exec_path field and use it when the probe selects the fallback; use only its dirname to locate trtexec_safe. Add a fallback test with a nonstandard executable basename.
This branch has not been deployed
7 participants
Previously there was a bug when using TrtExecBenchmark with the safety runtime and remoteAutoTuning that resulted in invalid latency measurements. This was due to using the local trtexec instead of the remote trtexec_safe.
This PR fixes this issue by using scp to push the resulting engine to the remote device and then remotely calling trtexec_safe to generate latency measurements.
This PR changes the behavior when specifying remote auto tuning such that an incorrect configuration will no longer gracefully fallback but rather fail hard and fast so that the user may correct the configuration.
Before:
After:
Security Exception
Requesting an exception since we need to call subprocesses to push data to a remote device for remote profiling.
Summary by CodeRabbit