feat(eagle3): add Eagle 3.1 training support - #610
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ChangesEagle3/PEagle norm_output and norm_before_fc
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The quality checks have failed. Please run |
Eagle 3.1 Evaluation ResultsTrained Eagle 3.1 drafters for Qwen/Qwen3-8B on 508k samples (magpie + ultrachat from Acceptance LengthsEvaluated on RedHatAI/speculator_benchmarks (9 subsets), k=5 draft tokens, temp=0.6, top_p=0.95, top_k=20, 80 requests per subset.
Eagle 3.1 (Llama-arch) is the best overall at +7.2% over Eagle 3 (Llama). Combining Qwen3-arch with Eagle 3.1 does not compound — gains are redundant since both norm strategies address the same underlying magnitude drift problem. Lowering LR from 5e-4 to 3e-4 for Eagle 3.1 (Qwen3) does not help, confirming the redundancy is architectural, not LR-related. Per-Position Acceptance Rates (averaged across subsets)
Eagle 3.1 (Llama) improvement grows with draft depth, while Qwen3-arch and Eagle 3.1 (Qwen3) plateau. fc_norm Ablation (per-hidden-state norm, paper approach)Ran the same Llama-arch, LR=3e-4 config but with
Note on bf16 training and norm weightsAll runs used However, this does not invalidate the results above. Any norm succeeded by a linear layer (FC, lm_head) is mathematically unaffected — the linear layer can absorb the diagonal scaling that the norm weights would have provided ( Training Config
|
I think I would prefer --norm-before-fc and --norm-output. Although if we're using llama arch should we default to eagle3.1? |
|
This pull request has merge conflicts that must be resolved before it can be |
|
Addressed the feedback — split
Both default to `True` when `--draft-arch llama` is used with eagle3/peagle, since the benchmarks show Eagle 3.1 (Llama) is uniformly better (+7.9% avg acceptance length). For qwen3 arch, both default to `False` (plain Eagle 3). Can be explicitly overridden with Also rebased onto latest main. vLLM side: |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Blocked by vllm-project/vllm#46488 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/speculators/convert/eagle/eagle3_converter.py`:
- Line 139: The `norm_before_fc` parameter on line 139 uses OR fallback logic
that prevents explicit False values from overriding checkpoint settings, while
`norm_before_residual` on line 138 and `norm_output` on line 140 use parameters
directly without fallback. Either align all three fields by removing the OR
fallback from the `norm_before_fc` assignment to match the other fields pattern,
or if checkpoint preservation is intentionally required only for
`norm_before_fc`, add a clear comment explaining why this field requires
different handling than `norm_before_residual` and `norm_output`.
In `@tests/integration/models/test_model_forward.py`:
- Around line 359-398: In the TestNormOutputParams class, the test methods
test_norm_output, test_norm_output_without_norm_before_fc, and
test_peagle_norm_before_fc all unpack variables from model calls but do not use
them. Prefix the unused variables draft_tokens and metrics with an underscore in
all three methods where they are unpacked (changing them to _draft_tokens and
_metrics) to follow Python conventions for intentionally unused variables and
silence the Ruff linter warnings.
🪄 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: 43f11dbb-a456-4322-8d73-ab5cf4a45d1c
📒 Files selected for processing (9)
docs/cli/train.mdscripts/train.pysrc/speculators/convert/eagle/eagle3_converter.pysrc/speculators/models/eagle3/config.pysrc/speculators/models/eagle3/core.pysrc/speculators/models/peagle/core.pytests/integration/conftest.pytests/integration/models/test_model_forward.pytests/unit/test_config.py
|
This pull request has merge conflicts that must be resolved before it can be |
…orm feedback) Eagle 3.1 addresses attention drift where hidden-state magnitude grows across speculation depths. Two changes: - RMSNorm before FC projection (norm_before_fc, already partially existed) - Post-norm hidden state feedback during TTT steps (use_post_norm_feedback) Also fixes P-EAGLE missing input_norm before FC and wires norm_before_fc through the converter and P-EAGLE's from_training_args. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Replace use_post_norm_feedback with a single eagle31 flag that enables both norm-before-FC and post-norm hidden state feedback. Keep norm_before_fc as a standalone flag for backward compat with existing gpt-oss checkpoints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
…onfig fields Replace the single --eagle31 flag with two independent flags: - --norm-before-fc: RMSNorm before FC projection (already existed) - --norm-output: post-norm hidden state feedback across TTT steps Default both to True when --draft-arch llama is used with eagle3/peagle, since Eagle 3.1 (Llama) showed +7.9% acceptance length improvement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
fynnsu
left a comment
There was a problem hiding this comment.
Generally looks good but I think the default chose logic feels a little complicated. Let's maybe wait for the Eagle3.1 + Qwen run (with the changed LR) to see if we can simplify this.
What do you think about I update it to remove the auto-defaulting logic, make both flags default to |
|
The quality checks have failed. Please run |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Remove auto-defaulting logic that enabled --norm-before-fc and --norm-output for llama arch. Both flags now default to False and use store_true instead of BooleanOptionalAction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
fef80ee to
72a8b7f
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com>
Eagle 3.1 (fc-norm) Evaluation Results — Qwen3-30B-A3BTrained Eagle 3.1 drafter for Qwen/Qwen3-30B-A3B using the per-layer FC normalization from orestis-z#1 ( Acceptance LengthsEvaluated on RedHatAI/speculator_benchmarks (9 subsets), k=5 draft tokens, temp=0.6, top_p=0.95, top_k=20, 80 requests per subset.
Per-Position Acceptance Rates (averaged across subsets)
Improvement grows with draft depth (pos 4: +13.7% relative). PlotsTraining Config
|
## Summary [PR #610](#610) benchmarks show that Eagle 3.1 with `--draft-arch llama --norm-before-fc --norm-output` gives **+7.2% average acceptance length** over Eagle 3 baselines (Qwen3-8B). This PR flips the CLI defaults so new training runs use these settings automatically. **Changes:** - `--draft-arch`: default `"qwen3"` → `"llama"` - `--norm-before-fc`: default `False` → `True`, switched to `BooleanOptionalAction` (adds `--no-norm-before-fc`) - `--norm-output`: default `False` → `True`, switched to `BooleanOptionalAction` (adds `--no-norm-output`) **Backward compatibility:** Config class defaults (`Eagle3SpeculatorConfig`) remain `False` — old checkpoints deserialize correctly. Users can restore Eagle 3 + qwen3 behavior with `--draft-arch qwen3 --no-norm-before-fc --no-norm-output`. ## Test plan - [x] `ruff check` passes on changed files - [x] `pytest tests/unit/train/test_draft_config_init.py` — all 36 tests pass - [x] `pytest tests/unit/test_config.py -k norm` — all 4 config roundtrip tests pass (config defaults unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…-project#692) ## Summary [PR vllm-project#610](vllm-project#610) benchmarks show that Eagle 3.1 with `--draft-arch llama --norm-before-fc --norm-output` gives **+7.2% average acceptance length** over Eagle 3 baselines (Qwen3-8B). This PR flips the CLI defaults so new training runs use these settings automatically. **Changes:** - `--draft-arch`: default `"qwen3"` → `"llama"` - `--norm-before-fc`: default `False` → `True`, switched to `BooleanOptionalAction` (adds `--no-norm-before-fc`) - `--norm-output`: default `False` → `True`, switched to `BooleanOptionalAction` (adds `--no-norm-output`) **Backward compatibility:** Config class defaults (`Eagle3SpeculatorConfig`) remain `False` — old checkpoints deserialize correctly. Users can restore Eagle 3 + qwen3 behavior with `--draft-arch qwen3 --no-norm-before-fc --no-norm-output`. ## Test plan - [x] `ruff check` passes on changed files - [x] `pytest tests/unit/train/test_draft_config_init.py` — all 36 tests pass - [x] `pytest tests/unit/test_config.py -k norm` — all 4 config roundtrip tests pass (config defaults unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…-project#692) ## Summary [PR vllm-project#610](vllm-project#610) benchmarks show that Eagle 3.1 with `--draft-arch llama --norm-before-fc --norm-output` gives **+7.2% average acceptance length** over Eagle 3 baselines (Qwen3-8B). This PR flips the CLI defaults so new training runs use these settings automatically. **Changes:** - `--draft-arch`: default `"qwen3"` → `"llama"` - `--norm-before-fc`: default `False` → `True`, switched to `BooleanOptionalAction` (adds `--no-norm-before-fc`) - `--norm-output`: default `False` → `True`, switched to `BooleanOptionalAction` (adds `--no-norm-output`) **Backward compatibility:** Config class defaults (`Eagle3SpeculatorConfig`) remain `False` — old checkpoints deserialize correctly. Users can restore Eagle 3 + qwen3 behavior with `--draft-arch qwen3 --no-norm-before-fc --no-norm-output`. ## Test plan - [x] `ruff check` passes on changed files - [x] `pytest tests/unit/train/test_draft_config_init.py` — all 36 tests pass - [x] `pytest tests/unit/test_config.py -k norm` — all 4 config roundtrip tests pass (config defaults unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Orestis Zambounis <orestis.zambounis@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eros483 <arnabmandal2912@gmail.com>




Purpose
Adds Eagle 3.1 training support with two config flags (matching vLLM's config fields):
--norm-before-fc: Apply a single RMSNorm to the concatenated target hidden states before the FC projection layer — i.e.Norm(concat(h₂, h₁₈, h₃₃)). This was already implemented on main for gpt-oss checkpoint compatibility; this PR adds the training CLI flag and wires it through P-EAGLE. Note: the original Eagle 3.1 paper specifies per-layer normalization (concat(Norm(h₂), Norm(h₁₈), Norm(h₃₃))), which corresponds to vLLM'sfc_normfield. Per-layer norm support (--fc-norm) will be added in a follow-up PR.--norm-output: Feed post-norm hidden states back across TTT steps to stabilize magnitude drift across speculation depths.Both flags default to
False(opt-in).Also fixes P-EAGLE missing
input_normbefore FC and wiresnorm_before_fc/norm_outputthrough the converter and P-EAGLE'sfrom_training_args.vLLM compatibility
norm_before_fcworks out of the box with vLLMnorm_outputrequires a one-line patch in vLLM'salgos.pyto propagate the config field (model code already supports it viagetattr): [Speculative Decoding] Propagate norm_output and fc_norm config for Eagle3 speculators vllm#46488Tests
test_norm_output— both flags enabled, verifies input_norm + forward + backwardtest_norm_output_without_norm_before_fc— norm_output alone (no input_norm)test_peagle_norm_before_fc— P-EAGLE with norm_before_fcChecklist