Skip to content

Fix verifier kwarg leaking into draft model constructors - #669

Merged
shanjiaz merged 2 commits into
mainfrom
fix/from-pretrained-verifier-kwarg-leak
Jun 26, 2026
Merged

Fix verifier kwarg leaking into draft model constructors#669
shanjiaz merged 2 commits into
mainfrom
fix/from-pretrained-verifier-kwarg-leak

Conversation

@rahul-tuli

@rahul-tuli rahul-tuli commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

⚠️ 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:

  • 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 Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

SpeculatorModel.from_pretrained now accepts a verifier argument, forwards verifier values through external checkpoint conversion, and only passes verifier into subclass construction when the target initializer accepts that keyword. A new utility inspects callables for keyword support.

Changes

Verifier routing in loading

Layer / File(s) Summary
API contract and docs
src/speculators/model.py
SpeculatorModel.from_pretrained adds a verifier parameter and documents its behavior during auto-conversion and config-driven loading.
External checkpoint verifier forwarding
src/speculators/model.py
The external-checkpoint conversion branch forwards a verifier path or id, and omits the value when the verifier is already a loaded model instance.
Callable kwarg detection and subclass forwarding
src/speculators/utils/util.py, src/speculators/model.py
callable_accepts_kwarg checks callable signatures, and from_pretrained only re-injects verifier into subclass kwargs when the selected __init__ accepts it.

Sequence Diagram(s)

sequenceDiagram
  participant U as User
  participant SPF as "SpeculatorModel.from_pretrained"
  participant ECP as "external-checkpoint conversion path"
  participant CAK as "callable_accepts_kwarg"
  participant TSI as "target subclass __init__"
  U->>SPF: verifier
  SPF->>ECP: verifier string/path or None
  SPF->>CAK: inspect TSI for verifier
  CAK-->>SPF: accepts / rejects
  SPF->>TSI: kwargs with verifier when accepted
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through loading paths so light,
With verifier tucked in just right.
If kwargs fit, I gave a cheer,
If not, I skipped that little peer.
Hop hop—model magic, bright!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: preventing verifier from leaking into draft model constructors.
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.
Description check ✅ Passed The description clearly matches the code changes, explaining the verifier loader fix and the new kwarg-handling helper.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/from-pretrained-verifier-kwarg-leak

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 force-pushed the fix/from-pretrained-verifier-kwarg-leak branch from 3dffa2e to f562cf2 Compare June 26, 2026 13:34
@rahul-tuli
rahul-tuli marked this pull request as ready for review June 26, 2026 13:36
@rahul-tuli rahul-tuli added bug Something isn't working ready This PR is ready for review labels Jun 26, 2026
@rahul-tuli rahul-tuli self-assigned this Jun 26, 2026
@rahul-tuli
rahul-tuli requested review from dsikka, fynnsu and shanjiaz June 26, 2026 13:56
shanjiaz and others added 2 commits June 26, 2026 11:03
Signed-off-by: shanjiaz <hezhao@redhat.com>
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>
@rahul-tuli
rahul-tuli changed the base branch from main to remove-eagle-converter June 26, 2026 15:19
@rahul-tuli
rahul-tuli force-pushed the fix/from-pretrained-verifier-kwarg-leak branch from f2a6e21 to b135ff3 Compare June 26, 2026 15:19
Base automatically changed from remove-eagle-converter to main June 26, 2026 18:00
@shanjiaz
shanjiaz enabled auto-merge (squash) June 26, 2026 20:49
@shanjiaz
shanjiaz merged commit 0451356 into main Jun 26, 2026
9 checks passed
@shanjiaz
shanjiaz deleted the fix/from-pretrained-verifier-kwarg-leak branch June 26, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready This PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants