[None][feat] Eagle: Norm before FC#12561
Conversation
Signed-off-by: Izzy Putterman <iputterman@nvidia.com>
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/models/modeling_speculative.py (1)
1-1:⚠️ Potential issue | 🟠 MajorAdd/refresh the NVIDIA copyright header for this modified source file.
This file was modified but has no copyright header at the top.
As per coding guidelines: “All TensorRT-LLM source files (.cpp, .h, .cu, .py) should contain an NVIDIA copyright header with the year of its latest meaningful modification.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/models/modeling_speculative.py` at line 1, This file (modeling_speculative.py) is missing the required NVIDIA copyright header; add the standard NVIDIA copyright header (with the latest modification year) at the very top of the file above the first import (above the existing "import inspect") so the header appears in the module-level comment block; ensure it follows the project's header template and includes the correct year and ownership text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/models/modeling_speculative.py`:
- Around line 564-567: The mismatch-path unconditionally calls self.model.fc
causing an AttributeError when fc is not created; update apply_eagle3_fc to
first check for the presence of the projection before invoking it (e.g., use
hasattr/getattr on self.model for "fc"), apply input_norm only if
self.model._norm_before_fc is true, and if fc is missing handle
gracefully—either skip projection (and document behavior) or raise a clear,
descriptive error mentioning missing fc—so the code does not crash in configs
where Eagle3DraftModel omits fc.
---
Outside diff comments:
In `@tensorrt_llm/_torch/models/modeling_speculative.py`:
- Line 1: This file (modeling_speculative.py) is missing the required NVIDIA
copyright header; add the standard NVIDIA copyright header (with the latest
modification year) at the very top of the file above the first import (above the
existing "import inspect") so the header appears in the module-level comment
block; ensure it follows the project's header template and includes the correct
year and ownership text.
🪄 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: Pro
Run ID: 2357c4a6-2969-40dc-8e0a-ed9536ddfa4e
📒 Files selected for processing (1)
tensorrt_llm/_torch/models/modeling_speculative.py
| if hidden_states.shape[-1] != expected_hidden_size: | ||
| if self.model._norm_before_fc: | ||
| hidden_states = self.model.input_norm(hidden_states) | ||
| hidden_states = self.model.fc(hidden_states) |
There was a problem hiding this comment.
Harden apply_eagle3_fc against missing fc in the mismatch path.
At Line 567, projection is unconditional once a hidden-size mismatch is detected. Since fc is created conditionally in Eagle3DraftModel.__init__, this can fail with an AttributeError in unsupported/edge configs.
Proposed hardening diff
expected_hidden_size = self.model.hidden_size
+ fc = getattr(self.model, "fc", None)
if hidden_states.shape[-1] != expected_hidden_size:
+ if fc is None:
+ raise RuntimeError(
+ "Hidden size mismatch requires Eagle3 FC projection, but `fc` is not initialized."
+ )
if self.model._norm_before_fc:
hidden_states = self.model.input_norm(hidden_states)
- hidden_states = self.model.fc(hidden_states)
+ hidden_states = fc(hidden_states)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/_torch/models/modeling_speculative.py` around lines 564 - 567,
The mismatch-path unconditionally calls self.model.fc causing an AttributeError
when fc is not created; update apply_eagle3_fc to first check for the presence
of the projection before invoking it (e.g., use hasattr/getattr on self.model
for "fc"), apply input_norm only if self.model._norm_before_fc is true, and if
fc is missing handle gracefully—either skip projection (and document behavior)
or raise a clear, descriptive error mentioning missing fc—so the code does not
crash in configs where Eagle3DraftModel omits fc.
|
PR_Github #40507 [ run ] triggered by Bot. Commit: |
|
PR_Github #40507 [ run ] completed with state |
2 participants
Summary by CodeRabbit
Description
Test Coverage
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)
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.