Skip to content

[None][fix] mpi_session: guard MpiPoolSession.shutdown against partial init#16613

Open
lowsfer wants to merge 1 commit into
NVIDIA:mainfrom
lowsfer:fix/mpi-pool-session-shutdown-partial-init
Open

[None][fix] mpi_session: guard MpiPoolSession.shutdown against partial init#16613
lowsfer wants to merge 1 commit into
NVIDIA:mainfrom
lowsfer:fix/mpi-pool-session-shutdown-partial-init

Conversation

@lowsfer

@lowsfer lowsfer commented Jul 20, 2026

Copy link
Copy Markdown
Member

Summary

MpiPoolSession.shutdown() reads self.n_workers (in a log line) and self._wait_shutdown unguarded, but it is reachable on a session that never completed __init__:

  • from __del__shutdown_abort()shutdown() on a partially-constructed object, and
  • from the released-session path, e.g. tests/unittest/executor/test_proxy_fast_death.py::test_pool_session_shutdown_never_blocks_after_release, which builds the object with MpiPoolSession.__new__(MpiPoolSession).

In those states the attributes are absent and shutdown() raises AttributeError: 'MpiPoolSession' object has no attribute 'n_workers' (then ... '_wait_shutdown'). The _pool_dead check immediately above already uses getattr(self, '_pool_dead', False) defensively for exactly this reason.

Fix

Guard both accesses with getattr(...), matching the existing _pool_dead style, so a released or partially-constructed session shuts down cleanly instead of raising.

Why main is currently red

This is a cross-PR merge skew, each half green on its own:

Neither failed in isolation; the combination on main fails deterministically. main has been red on test_pool_session_shutdown_never_blocks_after_release since #16312 landed, and the fix is a small robustness guard.

Test

test_proxy_fast_death.py now passes in full (23 passed, was 1 failed):

python -m pytest tests/unittest/executor/test_proxy_fast_death.py -q
# 23 passed

(No behavior change for fully-initialized sessions: n_workers / _wait_shutdown are always set by __init__; the guards only affect objects that bypass it.)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Improved formatting and organization of API documentation.
    • Clarified session initialization and external communication examples.
  • Bug Fixes

    • Improved shutdown logging when the worker count is unavailable.
    • Preserved existing worker cleanup and shutdown behavior.

…l init

MpiPoolSession.shutdown() is reachable from __del__ (via shutdown_abort)
and may run on a session that never completed __init__ -- e.g. after
release_exit_joins() marks the pool dead, or in the released-session
path exercised by test_proxy_fast_death.py::
test_pool_session_shutdown_never_blocks_after_release, which builds the
object with __new__. shutdown() read self.n_workers (log line) and
self._wait_shutdown unguarded, raising AttributeError, while the
_pool_dead check right above already used getattr defensively.

Guard both accesses with getattr, matching the existing _pool_dead
style, so a released/partially-constructed session shuts down without
crashing. This unbreaks a cross-PR merge-skew regression on main: the
unguarded self.n_workers log came from NVIDIA#16456 while the __new__-based
test came from NVIDIA#16312; each was green alone but the combination fails
deterministically. Also add the missing D205 docstring blank lines the
ruff-legacy gate flagged on the touched file. Verified: the whole
test_proxy_fast_death.py file now passes (23 passed, was 1 failed).

Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
@lowsfer
lowsfer requested a review from a team as a code owner July 20, 2026 09:28
@lowsfer
lowsfer requested review from Tabrizian and cascade812 July 20, 2026 09:28
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3bdebf1f-6d01-4d6e-be4e-8d51bfc3d85a

📥 Commits

Reviewing files that changed from the base of the PR and between b8604c4 and f036089.

📒 Files selected for processing (1)
  • tensorrt_llm/llmapi/mpi_session.py

📝 Walkthrough

Walkthrough

Changes

MPI session updates

Layer / File(s) Summary
Docstrings and shutdown logging
tensorrt_llm/llmapi/mpi_session.py
Docstrings were reformatted, and shutdown logging now uses a fallback worker count without changing pool teardown behavior.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: cascade812

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, follows the required ticket/type format, and matches the main fix in the PR.
Description check ✅ Passed The PR description covers the bug, fix, regression context, and test results, so it is mostly complete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands.

@lowsfer

lowsfer commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60349 [ run ] triggered by Bot. Commit: f036089 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60349 [ run ] completed with state SUCCESS. Commit: f036089
/LLM/main/L0_MergeRequest_PR pipeline #48692 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lowsfer

lowsfer commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60380 [ run ] triggered by Bot. Commit: f036089 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60380 [ run ] completed with state FAILURE. Commit: f036089
/LLM/main/L0_MergeRequest_PR pipeline #48719 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lowsfer

lowsfer commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60420 [ run ] triggered by Bot. Commit: f036089 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60420 [ run ] completed with state FAILURE. Commit: f036089
/LLM/main/L0_MergeRequest_PR pipeline #48757 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lowsfer

lowsfer commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #60600 [ run ] triggered by Bot. Commit: f036089 Link to invocation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants