Skip to content

remove eagle converter and reference - #672

Merged
shanjiaz merged 2 commits into
mainfrom
remove-eagle-converter
Jun 26, 2026
Merged

remove eagle converter and reference#672
shanjiaz merged 2 commits into
mainfrom
remove-eagle-converter

Conversation

@shanjiaz

@shanjiaz shanjiaz commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Deprecate old eagle converter pathway. It's incompatible with rest of our converters and we haven't seen enough community engagement.

Tests

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.

Signed-off-by: shanjiaz <hezhao@redhat.com>
@shanjiaz shanjiaz added the ready This PR is ready for review label Jun 26, 2026
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dceff167-e788-486f-b030-785ba8847d94

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR removes the legacy Eagle conversion/model surface and related tests, narrows convert_model to eagle3/mtp/dflash, and updates the Eagle package export to expose only Eagle3Converter.

Changes

Eagle conversion surface removal

Layer / File(s) Summary
Package export trim
src/speculators/convert/eagle/__init__.py
__all__ now exports only Eagle3Converter.
Conversion entrypoint narrowing
src/speculators/convert/entrypoints.py
Module docs, imports, convert_model typing, and runtime dispatch remove Eagle v1/v2/HASS support and keep eagle3, mtp, and dflash.

Possibly related PRs

  • vllm-project/speculators#549: Adjusts src/speculators/convert/eagle/eagle_converter.py, overlapping with this PR’s removal of the Eagle conversion surface.
  • vllm-project/speculators#617: Also changes src/speculators/convert/entrypoints.py around the DFlash conversion path and algorithm typing.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing the Eagle converter and its references.
Description check ✅ Passed The description is clearly related and explains the deprecation of the old Eagle converter pathway.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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
  • Commit unit tests in branch remove-eagle-converter

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.

@rahul-tuli
rahul-tuli self-requested a review June 26, 2026 15:14

@rahul-tuli rahul-tuli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Beautiful!

@rahul-tuli
rahul-tuli enabled auto-merge (squash) June 26, 2026 15:15
rahul-tuli added a commit that referenced this pull request Jun 26, 2026
build_draft_model passes verifier=... to from_pretrained for the
external-checkpoint auto-convert path. SpeculatorModel.from_pretrained
had no verifier parameter, so it stayed in **kwargs, was forwarded to
HF's from_pretrained, and reached the model constructor cls(config,
**kwargs). Draft models take only `config`, so this raised:

  TypeError: Eagle3DraftModel.__init__() got an unexpected keyword
  argument 'verifier'

Make verifier an explicit named parameter of from_pretrained so it is
consumed (never forwarded to the constructor) and used directly for
auto-conversion; forward it through the base-class re-dispatch alongside
t2d/d2t.

Stacked on #672 (eagle converter removal): with the attachment-style
EagleSpeculator gone, no model constructor needs verifier, so consuming
it at the loader boundary is sufficient.

Signed-off-by: Rahul-Tuli <rtuli@redhat.com>
@fynnsu

fynnsu commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Are there any docs/readme's linking to this stuff?

@shanjiaz

shanjiaz commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Are there any docs/readme's linking to this stuff?

@fynnsu looks like there's one that claude didn't catch, will remove that. Never mind, https://docs.vllm.ai/projects/speculators/en/latest/reference/speculators/convert/eagle/eagle_converter/ this seems to be auto-generated by mkdocstrings?

@shanjiaz
shanjiaz disabled auto-merge June 26, 2026 17:47
@shanjiaz
shanjiaz enabled auto-merge (squash) June 26, 2026 17:48
@shanjiaz
shanjiaz merged commit bcdc38a into main Jun 26, 2026
9 checks passed
@shanjiaz
shanjiaz deleted the remove-eagle-converter branch June 26, 2026 18:00
shanjiaz pushed a commit that referenced this pull request Jun 26, 2026
<!-- markdownlint-disable -->

> ⚠️ **DO NOT MERGE before #672.** This PR is stacked on #672 (remove
eagle converter) and its base is `remove-eagle-converter`. Merging this
first would pull in #672's changes prematurely and/or land against the
wrong base. Merge order: **#672 first, then this.** Once #672 merges, I
will rebase this onto `main`. The diff here is the 3-line loader change
only.

## Purpose

Fix `TypeError: Eagle3DraftModel.__init__() got an unexpected keyword
argument 'verifier'` raised during draft model construction in
`scripts/train.py --from-pretrained`.

This breaks the EAGLE3 finetuning smoke test:

```
tests/e2e/smoke/test_finetuning_sanity.py::test_finetuning_weight_sanity
TypeError: Eagle3DraftModel.__init__() got an unexpected keyword argument 'verifier'
```

`build_draft_model` passes `verifier=args.verifier_name_or_path` to
`from_pretrained` (for the external-checkpoint auto-convert path added
in #617). `SpeculatorModel.from_pretrained` had no `verifier` parameter,
so it stayed in `**kwargs`, was forwarded to HF's `from_pretrained`, and
reached the model constructor `cls(config, **kwargs)`. Draft models take
only `config` and reject it.

## Fix

Make `verifier` an explicit named parameter of
`SpeculatorModel.from_pretrained` so it is consumed at the loader
boundary (never forwarded into the constructor), used directly for
auto-conversion, and forwarded through the base-class re-dispatch
alongside `t2d`/`d2t`.

With the attachment-style `EagleSpeculator` removed in #672, no model
constructor takes `verifier` anymore, so consuming it is sufficient — no
per-model special-casing needed.

## Tests

- Failing test fixed:
`tests/e2e/smoke/test_finetuning_sanity.py::test_finetuning_weight_sanity`
(covered on CI).
- `make style` / `make quality`: pass.
- `pytest tests/unit/test_model.py
tests/unit/convert/test_entrypoints.py`: 20 passed.

## Checklist

I have filled in:

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

---------

Signed-off-by: shanjiaz <hezhao@redhat.com>
Signed-off-by: Rahul-Tuli <rtuli@redhat.com>
Co-authored-by: shanjiaz <hezhao@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready This PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants