Skip to content

Remove deprecated Eagle3 legacy model from converter#812

Open
guan404ming wants to merge 1 commit into
vllm-project:mainfrom
guan404ming:remove-eagle3-legacy-model
Open

Remove deprecated Eagle3 legacy model from converter#812
guan404ming wants to merge 1 commit into
vllm-project:mainfrom
guan404ming:remove-eagle3-legacy-model

Conversation

@guan404ming

@guan404ming guan404ming commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Purpose

Part of the legacy code removal item in #781.

  • Delete deprecated eagle3_legacy_model.py; its forward() was never implemented and it only served as a weight skeleton for the Eagle3 converter.
  • Convert with the training-side Eagle3DraftModel instead, following the DFlash converter pattern. Net -387 lines.
  • Side fix: the legacy skeleton hardcoded a single decoder layer, silently dropping layers.1.* weights of multi-layer checkpoints. Eagle3DraftModel builds all layers, so they are now preserved.

Tests

  • New unit test in test_eagle3_converter.py: converts a tiny 2-layer checkpoint and asserts the saved state dict keeps every decoder layer, contains only source-checkpoint keys, and round-trips values exactly.
  • tests/unit/convert/: 52 passed. make quality (ruff + mypy) passes.

Checklist

I have filled in:

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan/results, such as providing test command and pasting the results.
  • (Optional) The necessary documentation update.
  • I (a human) have written or reviewed the code in this pr to the best of my ability.

@coderabbitai

coderabbitai Bot commented Jul 18, 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: Pro

Run ID: cf03d131-c10d-4a7e-8cc6-8bf3cd14f1eb

📥 Commits

Reviewing files that changed from the base of the PR and between 1afb3b2 and fce748e.

📒 Files selected for processing (4)
  • src/speculators/convert/eagle/eagle3_converter.py
  • src/speculators/convert/eagle/eagle3_legacy_model.py
  • tests/integration/convert/test_eagle3.py
  • tests/unit/convert/test_eagle3_converter.py
💤 Files with no reviewable changes (1)
  • src/speculators/convert/eagle/eagle3_legacy_model.py

📝 Walkthrough

Walkthrough

Changes

The EAGLE-3 conversion flow now creates and validates Eagle3DraftModel checkpoints. It filters verifier-filled missing keys, saves only remapped input weights, removes the legacy speculator implementation, and updates unit and integration tests.

EAGLE-3 draft conversion

Layer / File(s) Summary
Converter model and call-path migration
src/speculators/convert/eagle/eagle3_converter.py
The converter uses Eagle3DraftModel, derives target vocabulary size from t2d, filters verifier-filled keys, and simplifies save/validation method signatures.
Checkpoint filtering and validation
src/speculators/convert/eagle/eagle3_converter.py, tests/unit/convert/test_eagle3_converter.py
Checkpoint saving retains remapped input weights, validation checks selected tensors for NaNs, and unit coverage verifies layer preservation and exact tensor values.
Legacy model and integration updates
src/speculators/convert/eagle/eagle3_legacy_model.py, tests/integration/convert/test_eagle3.py
The legacy Eagle3Speculator implementation and forward-pass helper are removed, while integration tests load converted checkpoints as Eagle3DraftModel instances.

Possibly related PRs

  • vllm-project/speculators#670: Adds Eagle3DraftModel and Eagle3SpeculatorConfig behavior that intersects with this converter’s draft-model checkpoint flow.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: removing the deprecated Eagle3 legacy model from the converter.
Description check ✅ Passed The description matches the PR scope by explaining the legacy model removal, the draft-model switch, and the added tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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

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.

❤️ Share

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

@guan404ming
guan404ming force-pushed the remove-eagle3-legacy-model branch from e85513b to 8c454f4 Compare July 18, 2026 09:16
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
@guan404ming
guan404ming force-pushed the remove-eagle3-legacy-model branch from 8c454f4 to fce748e Compare July 18, 2026 09:17
@guan404ming
guan404ming marked this pull request as ready for review July 18, 2026 09:22

@speculatorsbot speculatorsbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Clean removal of the deprecated legacy model, correctly replaces it with Eagle3DraftModel following the DFlash converter pattern. The side fix for multi-layer checkpoint preservation is correct and well-tested. Recommend approving.

🤖 Generated with Claude Code using the /pr-review skill

speculatorsbot

This comment was marked as duplicate.

@mergify

mergify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require approval from approved reviewers list 👀 reviews

🔴 Require approval from approved reviewers list

Waiting for any of

  • approved-reviews-by = dsikka
  • approved-reviews-by = fynnsu
  • approved-reviews-by = orestis-z
  • approved-reviews-by = rahul-tuli
  • approved-reviews-by = shanjiaz
This rule is failing.

All pull requests must have at least one approving review from a member of the approved reviewers list before merging.

  • any of:
    • approved-reviews-by = dsikka
    • approved-reviews-by = fynnsu
    • approved-reviews-by = orestis-z
    • approved-reviews-by = rahul-tuli
    • approved-reviews-by = shanjiaz

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