[https://nvbugs/6523880][fix] Restored the legacy-files.txt entry and regenerated the three derived… - #16943
[https://nvbugs/6523880][fix] Restored the legacy-files.txt entry and regenerated the three derived…#16943trtllm-agent wants to merge 1 commit into
legacy-files.txt entry and regenerated the three derived…#16943Conversation
…i_session D205 L0_PostMerge "Release Check" runs pre-commit over all files and failed on three hooks, from two independent regressions. 1. ruff + ruff-format on tests/integration/defs/accuracy/test_cli_flow.py PR NVIDIA#16612 dropped 537 entries from legacy-files.txt while removing legacy python relics. test_cli_flow.py is the only one of those 537 whose file still exists on disk, so dropping its entry deleted nothing -- it silently reclassified the file from Group B (yapf/isort/autoflake formatting plus supplemental ruff-legacy lint) into Group A (full ruff check + ruff format), a style it has never conformed to. That surfaced 4 ruff findings (I001 plus three E501 from the medusa/eagle choice-tree literals) and a whole-file ruff-format rewrite. Restored the legacy-files.txt entry and regenerated the derived configs with `python scripts/legacy_utils.py gen-configs`, which adds the path to ruff-legacy.toml, the managed pyproject.toml block and the managed .pre-commit-config.yaml block. The file is already clean under yapf, isort, autoflake and ruff-legacy, so its classification is consistent again with no formatting churn. 2. ruff-legacy on tensorrt_llm/llmapi/mpi_session.py PR NVIDIA#15908 added an env_overrides paragraph to MpiPoolSession.__init__, whose docstring opens directly with `"""Args:`. That gave the file a second D205 against a ratchet baseline of D205: 1. Rewrote the docstring with a summary line, a blank line and a Google-style Args: block, re-wrapped inside yapf's 80-column limit. The file is back to its baseline counts (D200: 1, D205: 1, D415: 1); the baseline itself is unchanged and no rule is disabled. Signed-off-by: handongl <handongl@nvidia.com>
WalkthroughChangesLegacy tooling coverage
MPI session documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
🤖 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 `@tensorrt_llm/llmapi/mpi_session.py`:
- Around line 248-256: Update the wait_shutdown documentation near
_wait_workers_exit() to state that shutdown waits up to the 30-second timeout
and may return early with a warning if workers remain alive. Clarify that this
is best-effort and callers must not assume worker GPU memory has been released
after the timeout.
🪄 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: 33f1fdef-15bb-404f-9bb3-b942379f8927
📒 Files selected for processing (5)
.pre-commit-config.yamllegacy-files.txtpyproject.tomlruff-legacy.tomltensorrt_llm/llmapi/mpi_session.py
| wait_shutdown: when True, ``shutdown()`` blocks until the spawned | ||
| worker processes have actually exited. | ||
| ``MPIPoolExecutor.shutdown`` returns at disconnect, but a | ||
| worker's GPU memory is only released when its process exits; | ||
| callers that start new GPU work right after ``shutdown()`` | ||
| (e.g. CI test infrastructure handing a pre-spawned pool to the | ||
| next test) race that release and can OOM. Off by default: | ||
| production teardown does not need the barrier and keeps its | ||
| current latency. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Document the shutdown timeout and best-effort behavior.
This says wait_shutdown=True blocks until workers have actually exited, but _wait_workers_exit() returns after its 30-second deadline even when a worker is still alive. Describe this as waiting up to the timeout, with an explicit warning/early return, otherwise callers may incorrectly assume GPU memory has been released before starting new work.
Based on the supplied implementation, _wait_workers_exit() is bounded by 30 seconds.
Suggested wording
- wait_shutdown: when True, ``shutdown()`` blocks until the spawned
- worker processes have actually exited.
+ wait_shutdown: when True, ``shutdown()`` waits for the spawned
+ worker processes to exit, up to the internal timeout.
+ If a worker remains alive after the timeout, shutdown logs a
+ warning and returns without guaranteeing that its GPU memory
+ has been released.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wait_shutdown: when True, ``shutdown()`` blocks until the spawned | |
| worker processes have actually exited. | |
| ``MPIPoolExecutor.shutdown`` returns at disconnect, but a | |
| worker's GPU memory is only released when its process exits; | |
| callers that start new GPU work right after ``shutdown()`` | |
| (e.g. CI test infrastructure handing a pre-spawned pool to the | |
| next test) race that release and can OOM. Off by default: | |
| production teardown does not need the barrier and keeps its | |
| current latency. | |
| wait_shutdown: when True, ``shutdown()`` waits for the spawned | |
| worker processes to exit, up to the internal timeout. | |
| If a worker remains alive after the timeout, shutdown logs a | |
| warning and returns without guaranteeing that its GPU memory | |
| has been released. |
🤖 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 `@tensorrt_llm/llmapi/mpi_session.py` around lines 248 - 256, Update the
wait_shutdown documentation near _wait_workers_exit() to state that shutdown
waits up to the 30-second timeout and may return early with a warning if workers
remain alive. Clarify that this is best-effort and callers must not assume
worker GPU memory has been released after the timeout.
|
/bot run --disable-fail-fast |
|
PR_Github #62195 [ run ] triggered by Bot. Commit: |
|
PR_Github #62195 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62320 [ run ] triggered by Bot. Commit: |
|
PR_Github #62320 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62352 [ run ] triggered by Bot. Commit: |
|
PR_Github #62352 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62429 [ run ] triggered by Bot. Commit: |
|
PR_Github #62429 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62477 [ run ] triggered by Bot. Commit: |
7 participants
Summary
test_cli_flow.pyfromlegacy-files.txtwhile the file still exists, silently promoting it from Group B to Group A (fullruff check/format) which it never conformed to; independently PR [None][test] Add opt-in background prefetch of test MPI sessions and model page cache #15908 added a secondD205toMpiPoolSession.__init__past its ratchet baseline of 1.legacy-files.txtentry and regenerated the three derived configs viascripts/legacy_utils.py gen-configs(no hand edits); rewrote the__init__docstring with a summary line, blank line, and Google-styleArgs:block inside yapf's 80-col limit — no baseline loosened, no rule disabled.Test plan
Links
Dev Engineer Review
tests/integration/defs/accuracy/test_cli_flow.pyto the legacy classification and regenerated all related Ruff/pre-commit configuration entries consistently.MpiPoolSession.__init__documentation into a compliant Google-styleArgs:section, clarifying shutdown and environment override behavior without changing executable logic or API signatures.QA Engineer Review