fix(autotune): pre-check remote board connectivity before benchmark - #2078
fix(autotune): pre-check remote board connectivity before benchmark#2078willg-nv wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 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
RemoteBoard-->>TrtExecBenchmark: Unreachable
TrtExecBenchmark-->>AutotuneWorkflow: Raise RemoteConnectionError
AutotuneWorkflow->>AutotuneWorkflow: Save autotuner checkpoint
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. |
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>
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
| parsed = urllib.parse.urlparse(config_value) | ||
| hostname = parsed.hostname | ||
| if not hostname: | ||
| return | ||
|
|
||
| port = parsed.port | ||
| if port is None: | ||
| port = _DEFAULT_PORTS.get(parsed.scheme, 22) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize and validate the remote URI.
The documented CLI command reaches this code with quotes in the --remoteAutoTuningConfig value because run_autotune() uses str.split(). urlparse() then produces no hostname, and Line 88 returns without a TCP check. Strip matching quotes before parsing. Raise RemoteConnectionError when the host is missing or the port is invalid.
Based on supplied workflow and documentation context, run_autotune() uses str.split() for the documented remote configuration value.
🤖 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 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.
| _check_remote_connectivity(self._base_cmd, retries=self._remote_connection_retries) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Save state for baseline and final connectivity failures.
Line 329 runs for baseline, per-scheme, and final measurements. The supplied region_pattern_autotuning_workflow() catches RemoteConnectionError only around per-scheme measurements. A baseline failure exits without saving state. A final failure can exit after committing the final region but before persisting that commit. Catch this exception at a scope that covers every benchmark_onnx_model() call, save the state, then re-raise it.
Based on supplied workflow context, the current state-save handler surrounds only per-scheme measurements.
🤖 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 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.
|
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. |
1 participant
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