Skip to content

[https://nvbugs/6478723][fix] Rename forward -> _forward_impl AND stash the extra dynamic-tree state on… - #16733

Open
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6478723
Open

[https://nvbugs/6478723][fix] Rename forward -> _forward_impl AND stash the extra dynamic-tree state on…#16733
trtllm-agent wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6478723

Conversation

@trtllm-agent

@trtllm-agent trtllm-agent commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: MTPEagleDynamicTreeWorker overrode SpecWorkerBase.forward, which init_subclass forbids (nvbugs/6442074) — subclasses must implement _forward_impl so the base-class forward can guarantee spec-dec attn-metadata cleanup on failure. Class-creation TypeError blocked pytest collection.
  • Fix: Rename forward -> _forward_impl AND stash the extra dynamic-tree state on self, then override _ensure_spec_dec_state_restored to restore it — matching the PARD/DFlash pattern. This addresses both the immediate collection failure and closes the state-leak gap the base class explicitly documents for subclasses with extra transient state.
  • Automated fix generated by repair-bot

Test plan

  • Verify fix on the same GPU type as the original failure
  • Check for regressions in related tests

Links

Dev Engineer Review

  • Renamed forward to _forward_impl to comply with SpecWorkerBase.__init_subclass__, allowing centralized failure cleanup.
  • Preserved the existing profiler label and implementation signature.
  • Added restoration of all_rank_num_tokens and force_prepare_spec_dec_tree_mask after tolerated failures, preventing dynamic-tree state leakage.
  • Clears the saved snapshot after restoration.
  • No configuration or test-list changes.

QA Engineer Review

No test changes.

…orward_impl and route cleanup through base class

MTPEagleDynamicTreeWorker (added by NVIDIA#15582) overrode SpecWorkerBase.forward
directly, which SpecWorkerBase.__init_subclass__ (nvbugs/6442074, NVIDIA#16382)
forbids: subclasses must implement _forward_impl so the base-class forward
can guarantee spec-dec attn-metadata cleanup on failure. The class-creation
TypeError blocked pytest collection for every test in
tests/integration/defs/accuracy/test_llm_api_pytorch.py.

Fix:
  * Rename MTPEagleDynamicTreeWorker.forward -> _forward_impl. Signature
    already matches parent MTPEagleWorker._forward_impl (eagle3.py:712),
    SpecWorkerBase.forward delegates via *args/**kwargs, and the
    @nvtx_range("mtp_dyn.forward") decorator is preserved for profiler-
    label stability.
  * Stash the extra dynamic-tree-only attn_metadata state
    (all_rank_num_tokens, force_prepare_spec_dec_tree_mask) on self and
    override _ensure_spec_dec_state_restored so a tolerated forward
    failure between _prepare_attn_metadata_for_spec_dec and
    _restore_attn_metadata_from_spec_dec doesn't leak the pre-draft-loop
    state into subsequent iterations. Same pattern PARD/DFlash use.
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

MTPEagleDynamicTreeWorker now snapshots additional speculative decoding attention state before dynamic-tree execution and restores it during cleanup. Its execution method is renamed from forward to _forward_impl.

Changes

Dynamic tree state handling

Layer / File(s) Summary
State capture and execution entry point
tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py
Adds saved extra state for attention metadata, renames the worker entry point to _forward_impl, and snapshots token counts and the tree mask before draft-tree processing.
Cleanup state restoration
tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py
Restores the saved metadata through _ensure_spec_dec_state_restored and the draft-loop cleanup path, then clears the snapshot.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: zhaoyangwang-nvidia

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is useful, but it does not follow the required template sections for Description, Test Coverage, and PR Checklist. Add the missing template sections: @coderabbitai summary, Description, Test Coverage, and PR Checklist.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is clearly about the same fix: renaming forward and restoring extra dynamic-tree state.
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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py (1)

238-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add type annotations to these overrides. tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py#L238-L250 should annotate both metadata parameters and return None; tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py#L661-L671 should annotate its parameters and return type consistently with the worker contract.

🤖 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/_torch/speculative/mtp_dynamic_tree.py` around lines 238 - 250,
Add type annotations to the _ensure_spec_dec_state_restored override parameters
using the appropriate attention and speculative metadata types, and annotate its
return as None. Also update the override at
tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py lines 661-671 with parameter
and return annotations consistent with the worker contract.

Source: Coding guidelines

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

Nitpick comments:
In `@tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py`:
- Around line 238-250: Add type annotations to the
_ensure_spec_dec_state_restored override parameters using the appropriate
attention and speculative metadata types, and annotate its return as None. Also
update the override at tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py lines
661-671 with parameter and return annotations consistent with the worker
contract.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1ce8df88-e0ff-4a29-9d1e-ea3a3c7469a9

📥 Commits

Reviewing files that changed from the base of the PR and between 9cd6a49 and 2872dcb.

📒 Files selected for processing (1)
  • tensorrt_llm/_torch/speculative/mtp_dynamic_tree.py

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.

1 participant