Skip to content

[None][fix] Drop the unbound is_idle guard left in the idle disagg CTX reap - #18263

Merged
tburt-nv merged 1 commit into
NVIDIA:mainfrom
pranav-nvidia:fix-is-idle-unbound
Aug 26, 2026
Merged

[None][fix] Drop the unbound is_idle guard left in the idle disagg CTX reap#18263
tburt-nv merged 1 commit into
NVIDIA:mainfrom
pranav-nvidia:fix-is-idle-unbound

Conversation

@pranav-nvidia

@pranav-nvidia pranav-nvidia commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Description

#17324 removed the is_idle parameter from PyExecutor._check_disagg_transfer_progress_when_idle, along with both call-site arguments and the local_need_ctx_check = is_idle or ... expression that consumed it. One read of is_idle was left behind:

            if (is_idle and self._dist_size(self.dist, "world_size") == 1 and
                    self.async_transfer_manager.has_any_inflight_requests()):
                self._check_disagg_ctx_cache_transfer_status(0)

The method now takes only self, so this raises NameError on the path it guards: synchronous (non-async) GEN transfer, single-rank world, outside the gen-only-no-context benchmark. Because the executor loop runs on PyExecutor.worker_thread, an exception escaping there is broadcast to every pending request rather than to one.

The guard has no value to restore. Both call sites invoke the method unconditionally, and the removed expression had already folded is_idle away, so dropping the term restores the behaviour #17324 intended.

The same commit left test_sync_single_rank_ctx_reaps_idle_transfer calling the old five-argument signature, so it fails with TypeError: ... got an unexpected keyword argument 'num_fitting_reqs'. The other six call sites in that file were already updated; this one was missed. Its assertion — _check_disagg_ctx_cache_transfer_status.assert_called_once_with(0) — is what the product fix restores.

This is also a lint regression: ruff-legacy reports F821 Undefined name 'is_idle' against a baseline recording only E712/E721 for this file, so Pre-commit Check fails for any PR that touches py_executor.py and re-runs after #17324 landed.

Test Coverage

tests/unittest/_torch/executor/test_py_executor.py — 149 passed (was 148 passed, 1 failed).

pre-commit run ruff-legacy --files tensorrt_llm/_torch/pyexecutor/py_executor.py passes; it fails on main.

PR Checklist

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

Dev Engineer Review

  • Updated _check_disagg_transfer_progress_when_idle to remove the unbound is_idle reference.
  • Single-rank synchronous GEN transfer cleanup now runs when transfers are in flight.
  • The change prevents a NameError and resolves the related ruff-legacy failure.
  • No public API, configuration, or test-list changes were introduced.

QA Engineer Review

  • Modified the single-rank synchronous idle-transfer test in tests/unittest/_torch/executor/test_py_executor.py.
  • The test now uses the method’s current default-only signature.
  • No test function was added or removed.
  • The existing test continues to cover the corrected single-rank CTX transfer path.
  • Reported coverage: 149 tests passed.
  • Verdict: sufficient.

…X reap

NVIDIA#17324 removed the is_idle parameter, both call-site arguments and the
local_need_ctx_check use, but left one read behind, so the single-rank CTX
reap path raises NameError. The same commit left the test calling the old
five-argument signature, which fails collection-time with TypeError.

Both call sites invoke the method unconditionally, so the guard has no
value to restore.

Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com>
@pranav-nvidia
pranav-nvidia marked this pull request as ready for review August 26, 2026 19:20
@pranav-nvidia
pranav-nvidia requested review from a team as code owners August 26, 2026 19:20
@coderabbitai

coderabbitai Bot commented Aug 26, 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: 4e4ee7b5-6c74-4205-889e-d72f58e8e084

📥 Commits

Reviewing files that changed from the base of the PR and between 0c68b4f and 0176c11.

📒 Files selected for processing (2)
  • tensorrt_llm/_torch/pyexecutor/py_executor.py
  • tests/unittest/_torch/executor/test_py_executor.py

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.


Walkthrough

The executor now checks single-rank context-transfer progress regardless of idle state. The related test uses the method’s current default-only signature and keeps its existing assertions.

Changes

Disaggregated transfer progress

Layer / File(s) Summary
Transfer reaping and test update
tensorrt_llm/_torch/pyexecutor/py_executor.py, tests/unittest/_torch/executor/test_py_executor.py
Single-rank context-transfer cleanup no longer requires an idle executor. The test removes obsolete keyword arguments and retains its assertions.

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

Merge Risk: ⚪ Minimal · up to 0176c

This localized change removes the stale idle-transfer guard and restores the affected test, with the reported unit and lint checks passing; no actionable merge-blocking risk remains after normal review.

Suggested reviewers: juney-nvidia

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main fix: removing the remaining unbound is_idle guard from the idle disaggregated CTX reap path. It uses the required [None][fix] format and is concise.
Description check ✅ Passed The description explains the root cause, affected execution path, code change, test update, lint impact, and validation results. It includes the required Description, Test Coverage, and PR Checklist s…
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.
Full details: Description check

Explanation

The description explains the root cause, affected execution path, code change, test update, lint impact, and validation results. It includes the required Description, Test Coverage, and PR Checklist sections.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@Tabrizian

Copy link
Copy Markdown
Member

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69517 [ run ] triggered by Bot. Commit: 0176c11 Link to invocation

@tburt-nv

Copy link
Copy Markdown
Collaborator

/bot skip --comment "main is failing pre-commit already"

@tburt-nv
tburt-nv enabled auto-merge (squash) August 26, 2026 20:24
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69528 [ skip ] triggered by Bot. Commit: 0176c11 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69517 [ run ] completed with state ABORTED. Commit: 0176c11

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #69528 [ skip ] completed with state SUCCESS. Commit: 0176c11
Skipping testing for commit 0176c11

Link to invocation

@tburt-nv
tburt-nv merged commit 599448d into NVIDIA:main Aug 26, 2026
14 checks passed
pranav-nvidia added a commit to pranav-nvidia/TensorRT-LLM that referenced this pull request Aug 26, 2026
Picks up NVIDIA#18263, which removes the unbound is_idle read that was failing
this branch's Pre-commit Check.

Signed-off-by: Pranav Shrestha <254760092+pranav-nvidia@users.noreply.github.com>
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.

6 participants