Skip to content

[TRTLLM-13409][fix] Count async KV completions as benchmark fill progress - #18456

Open
qiaoxj07 wants to merge 1 commit into
NVIDIA:mainfrom
qiaoxj07:codex/fix-async-disagg-fill-progress
Open

[TRTLLM-13409][fix] Count async KV completions as benchmark fill progress#18456
qiaoxj07 wants to merge 1 commit into
NVIDIA:mainfrom
qiaoxj07:codex/fix-async-disagg-fill-progress

Conversation

@qiaoxj07

@qiaoxj07 qiaoxj07 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Dev Engineer Review

  • Updated benchmark disaggregated fill progress tracking to count both synchronous and asynchronous GEN KV completions.
  • Supports Python V2 completion IDs and C++ request-state-only completion reporting.
  • Propagates the combined progress signal through the existing model-parallel all-gather.
  • Renamed progress state to use generation-transfer terminology.
  • No public API, configuration, or non-benchmark execution changes found.
  • Formatting, linting, syntax, and test-list validation passed.
  • Full pytest collection remains deferred because local runtime dependencies are missing.

QA Engineer Review

  • Modified tests/unittest/_torch/executor/test_benchmark_disagg.py:
    • Updated benchmark disaggregation progress tracking to use GenTransferStatus.
    • Added parameterized coverage for Python and C++ completion-reporting contracts.
    • Added retry coverage after generation-transfer progress.
    • Added coverage for resetting the fill-stall watchdog after asynchronous completion.
  • Modified tests/unittest/_torch/executor/test_py_executor.py:
    • Updated synchronous disaggregated transfer tests to use _disagg_gen_transfer_made_progress.
  • No tests/integration/test_lists/, test-db/, qa/, or waives.txt changes were reported.
  • The modified test functions are not listed in integration test-list files.
  • Verdict: sufficient.

Description

#17202 added a no-progress deadline to the benchmark-disaggregated fill gate, but the executor only fed synchronous receive completions into that deadline. With generation-transfer overlap enabled, asynchronous requests can complete steadily while the watchdog sees no progress and eventually terminates a healthy large fill after the default 600-second window.

This change records synchronous and asynchronous GEN KV completions in one per-iteration progress signal. It supports both transceiver contracts on current main: Python V2 completion IDs and the C++ runtime's request-state-only completion reporting. The existing model-parallel all-gather propagates that signal before pacing and stall checks, so this adds no collective and does not change non-benchmark execution or public APIs.

Related: #17202, #18178.

Test Coverage

  • Added a parameterized CPU regression that covers both Python completion-ID reporting and C++ state-mutation-only reporting.
  • The regression keeps the fill gate incomplete with an already-expired stall window, then verifies that a real async completion resets the watchdog and avoids the idle polling sleep.
  • Updated the existing sync completion tests and model-parallel peer-progress coverage for the generalized signal.
  • pre-commit passed for all changed files, including isort, YAPF, Ruff, Ruff format, codespell, and repository policy hooks.
  • Python syntax compilation and test-list AST validation passed. Full pytest collection is deferred to CI because the local macOS Python 3.12 tooling environment does not contain the TensorRT-LLM/PyTorch runtime dependencies.

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • 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.

…ress

Signed-off-by: Xianjie <5410381+qiaoxj07@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 31, 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: 8ff67542-3f9e-4781-8dcb-8b7c444897ec

📥 Commits

Reviewing files that changed from the base of the PR and between 4c2ba54 and 7631710.

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

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


Walkthrough

Benchmark disaggregated fill tracking now uses generation-transfer state names. The fill gate records progress from Python transceiver completion IDs and C++ request-state transitions. Tests cover retry behavior and watchdog resets for both runtime paths.

Changes

Disaggregated generation transfer

Layer / File(s) Summary
Progress state and fill gate
tensorrt_llm/_torch/pyexecutor/py_executor.py
Renames benchmark progress state for generation transfers. The fill gate consumes aggregated progress and resets it between scheduling passes.
Completion detection
tensorrt_llm/_torch/pyexecutor/py_executor.py
Records progress from synchronous completion states, Python transceiver completion IDs, and C++ generation-transfer state transitions.
Retry and runtime coverage
tests/unittest/_torch/executor/test_benchmark_disagg.py, tests/unittest/_torch/executor/test_py_executor.py
Updates progress-state assertions and adds retry, watchdog, and Python/C++ completion-path coverage.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 76317

The change broadens benchmark fill-progress tracking to include asynchronous KV completions without altering non-benchmark behavior or public APIs. No actionable merge-blocking risk remains beyond normal checks and 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 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 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 fix: counting asynchronous KV completions as benchmark fill progress. It uses the required ticket and type format and is concise.
Description check ✅ Passed The description explains the issue, solution, scope, compatibility considerations, test coverage, validation results, and related issues. It also includes the required checklist and marks the final re…
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 issue, solution, scope, compatibility considerations, test coverage, validation results, and related issues. It also includes the required checklist and marks the final review item as complete.

  • 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.

@qiaoxj07

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

1 similar comment
@qiaoxj07

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70387 [ run ] triggered by Bot. Commit: 7631710 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70387 [ run ] completed with state SUCCESS. Commit: 7631710
/LLM/main/L0_MergeRequest_PR pipeline #57617 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

@qiaoxj07

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70432 [ run ] triggered by Bot. Commit: 7631710 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70432 [ run ] completed with state SUCCESS. Commit: 7631710
/LLM/main/L0_MergeRequest_PR pipeline #57659 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

@qiaoxj07

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

1 similar comment
@qiaoxj07

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70471 [ run ] triggered by Bot. Commit: 7631710 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70471 [ run ] completed with state FAILURE. Commit: 7631710
/LLM/main/L0_MergeRequest_PR pipeline #57694 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

@qiaoxj07

qiaoxj07 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70510 [ run ] triggered by Bot. Commit: 7631710 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #70510 [ run ] completed with state FAILURE. Commit: 7631710
/LLM/main/L0_MergeRequest_PR pipeline #57728 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

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