fix(autotune): pre-check remote board connectivity before benchmark - #2078
fix(autotune): pre-check remote board connectivity before benchmark#2078willg-nv wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughONNX Autotune checks remote board connectivity before each ChangesRemote autotuning connection handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant AutotuneWorkflow
participant TrtExecBenchmark
participant RemoteBoard
participant trtexec
CLI->>AutotuneWorkflow: Pass remote_connection_retries
AutotuneWorkflow->>TrtExecBenchmark: Initialize benchmark
TrtExecBenchmark->>RemoteBoard: Retry TCP connection
RemoteBoard-->>TrtExecBenchmark: Connection result
TrtExecBenchmark->>trtexec: Run benchmark after successful check
trtexec-->>TrtExecBenchmark: Benchmark result or failure
TrtExecBenchmark->>RemoteBoard: Recheck connectivity after failure
RemoteBoard-->>TrtExecBenchmark: Unreachable
TrtExecBenchmark-->>AutotuneWorkflow: Raise RemoteConnectionError
AutotuneWorkflow->>AutotuneWorkflow: Save autotuner state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
f95be17 to
7ad4729
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/onnx/quantization/autotune/__main__.py`:
- Around line 318-324: Validate the remote_connection_retries argument at the
CLI boundary where trt_group defines it, rejecting values below 1 and values
above a documented finite maximum. Add the allowed upper limit to the argument
help text and ensure the validated value is what _try_connect uses, preventing
zero-attempt behavior and excessive retry delays.
In `@modelopt/onnx/quantization/autotune/workflows.py`:
- Around line 338-345: Update benchmark_onnx_model so its generic exception
handling does not swallow RemoteConnectionError: add a dedicated handler that
re-raises it before the fallback handler. Preserve the existing float("inf")
behavior for other benchmark failures, allowing the workflow handler around
benchmark_onnx_model to save state and exit on remote connection loss.
🪄 Autofix
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: 3e201cba-baed-4f7c-b784-5ba792d08bd9
📒 Files selected for processing (5)
CHANGELOG.rstmodelopt/onnx/quantization/autotune/__main__.pymodelopt/onnx/quantization/autotune/benchmark.pymodelopt/onnx/quantization/autotune/common.pymodelopt/onnx/quantization/autotune/workflows.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
7ad4729 to
3eee981
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelopt/onnx/quantization/autotune/benchmark.py (1)
230-247: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
remote_connection_retriesat the public API boundary.A value of
0or less makes_try_connect()execute no attempts and returnNone. The benchmark then treats the remote board as reachable. A very large value can block the workflow for repeated five-second connection timeouts and retry delays. Reject values below1and enforce a documented upper bound before storing this value.As per path instructions, validate remote host/port and retry inputs, and enforce sensible timeouts and limits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 230 - 247, Validate remote_connection_retries in the benchmark constructor before assigning it to _remote_connection_retries: reject values below 1 and values above a documented sensible maximum, using the project’s existing validation/error conventions. Update the parameter documentation to state the accepted range, while preserving the retry behavior for valid values.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 329-330: Update region_pattern_autotuning_workflow so
RemoteConnectionError handling surrounds every benchmark_onnx_model call,
including baseline and final measurements; save the current autotuning state in
that handler before re-raising the exception, while preserving existing
per-scheme behavior.
- Around line 85-92: Update the remote URI handling in run_autotune() around
urllib.parse.urlparse to strip matching surrounding quotes from config_value
before parsing, then validate the parsed host and port. Raise
RemoteConnectionError when the hostname is missing or accessing the port
identifies an invalid port, while preserving the existing default-port lookup
for valid URIs.
---
Outside diff comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 230-247: Validate remote_connection_retries in the benchmark
constructor before assigning it to _remote_connection_retries: reject values
below 1 and values above a documented sensible maximum, using the project’s
existing validation/error conventions. Update the parameter documentation to
state the accepted range, while preserving the retry behavior for valid values.
🪄 Autofix
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: ffdbc6f1-6b9f-4123-86d2-8e22b53519ae
📒 Files selected for processing (6)
CHANGELOG.rstdocs/source/guides/9_autotune.rstmodelopt/onnx/quantization/autotune/__main__.pymodelopt/onnx/quantization/autotune/benchmark.pymodelopt/onnx/quantization/autotune/common.pymodelopt/onnx/quantization/autotune/workflows.py
🚧 Files skipped from review as they are similar to previous changes (4)
- modelopt/onnx/quantization/autotune/common.py
- modelopt/onnx/quantization/autotune/main.py
- CHANGELOG.rst
- modelopt/onnx/quantization/autotune/workflows.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modelopt/onnx/quantization/autotune/benchmark.py (2)
73-83: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject a remote configuration flag with no value.
If
trtexec_argsends with--remoteAutoTuningConfig, Line 78 does not setconfig_valueand Line 82 returns. The malformed flag remains in_base_cmd.trtexeccan then fail, and_benchmark_failed()can classify it as a scheme failure instead of preserving autotuner state.Raise
RemoteConnectionErrorwhen the flag has no following value.As per path instructions, validate untrusted connection parameters and preserve remote failure handling.
Proposed fix
- elif arg == "--remoteAutoTuningConfig" and i + 1 < len(trtexec_args): - config_value = trtexec_args[i + 1] - break + elif arg == "--remoteAutoTuningConfig": + if i + 1 >= len(trtexec_args): + raise RemoteConnectionError( + "--remoteAutoTuningConfig requires a remote URI value" + ) + config_value = trtexec_args[i + 1] + break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 73 - 83, Update the remote configuration argument parsing in the benchmark setup to raise RemoteConnectionError when --remoteAutoTuningConfig appears without a following value, rather than returning and leaving the malformed flag in _base_cmd. Preserve existing handling for inline and separate valid values and ensure the error follows the existing remote failure path.Source: Path instructions
237-254: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
remote_connection_retriesinTrtExecBenchmark.
_try_connectperforms zero attempts for values below1and accepts values above10. Enforce1 <= remote_connection_retries <= 10for direct API callers, not only in the CLI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/onnx/quantization/autotune/benchmark.py` around lines 237 - 254, Validate remote_connection_retries in TrtExecBenchmark.__init__ before assigning it to _remote_connection_retries, requiring an integer value from 1 through 10 inclusive. Reject values outside this range for direct API callers while preserving the existing retry behavior for valid inputs.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 89-97: Update the remote URI validation errors around parsed.port
and the hostname check to avoid including config_value, which may contain
credentials. Build messages using only the URI scheme, hostname, and port, while
preserving the existing RemoteConnectionError types and exception chaining for
invalid ports.
In `@modelopt/onnx/quantization/autotune/workflows.py`:
- Around line 298-303: Update all three exception handlers around the baseline,
tuning, and final benchmark flows to log the saved checkpoint path after
autotuner.save_state(str(state_path)) succeeds and before re-raising. Use
state_path in the message so users can locate the resume checkpoint, while
preserving the existing save-and-reraise behavior.
---
Outside diff comments:
In `@modelopt/onnx/quantization/autotune/benchmark.py`:
- Around line 73-83: Update the remote configuration argument parsing in the
benchmark setup to raise RemoteConnectionError when --remoteAutoTuningConfig
appears without a following value, rather than returning and leaving the
malformed flag in _base_cmd. Preserve existing handling for inline and separate
valid values and ensure the error follows the existing remote failure path.
- Around line 237-254: Validate remote_connection_retries in
TrtExecBenchmark.__init__ before assigning it to _remote_connection_retries,
requiring an integer value from 1 through 10 inclusive. Reject values outside
this range for direct API callers while preserving the existing retry behavior
for valid inputs.
🪄 Autofix
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: c53cc04b-6637-47ba-8028-ea8992e5e0dd
📒 Files selected for processing (2)
modelopt/onnx/quantization/autotune/benchmark.pymodelopt/onnx/quantization/autotune/workflows.py
When using --remoteAutoTuningConfig, test TCP connectivity to the remote board before each trtexec invocation. If unreachable after configurable retries (--remote_connection_retries, default 3), save state and exit cleanly instead of running trtexec which would fail and permanently mark schemes as errored in autotune_states.yaml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Will Guo <willg@nvidia.com>
- Strip matching surrounding quotes from config_value before urlparse (handles embedded quotes from --remoteAutoTuningConfig=\"...\") - Raise RemoteConnectionError on missing hostname or invalid port instead of silently returning - Add _benchmark_failed() to re-check connectivity after trtexec failure; raises RemoteConnectionError if board dropped mid-execution so the scheme is not incorrectly recorded as inf - Wrap baseline and final benchmark_onnx_model calls with RemoteConnectionError handling that saves autotuner state before re-raising Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Will Guo <willg@nvidia.com>
…sume The parser doesn't have a --resume option; state is detected automatically on re-run. Update the user-facing message accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Will Guo <willg@nvidia.com>
a219970 to
c045c3d
Compare
2 participants
fix(autotune): pre-check remote board connectivity before benchmark
Summary
When using
--remoteAutoTuningConfig, a lost connection to the remote boardcauses trtexec to fail. The failure gets recorded as
error: trueinautotune_states.yaml, permanently skipping those schemes on restart. This isincorrect — the error is transient (network issue), not a property of the
scheme itself.
This PR adds a TCP connectivity pre-check before each trtexec invocation. If
the board is unreachable after configurable retries, the autotuner saves state
and exits cleanly instead of poisoning the state file.
Changes
common.py: AddRemoteConnectionErrorexception (subclass ofAutotunerError)benchmark.py: Add_check_remote_connectivity()and_try_connect()helpers;call pre-check in
TrtExecBenchmark.run()before launching trtexecworkflows.py: CatchRemoteConnectionErrorin per-scheme loop, save statebefore re-raising
__main__.py: Add--remote_connection_retriesCLI argument (default: 3)CHANGELOG.rst: Add bug fix entryBehavior
error: trueerror: trueNew CLI option
Summary by CodeRabbit