Skip to content

fix dflash converter issues - #662

Merged
rahul-tuli merged 4 commits into
mainfrom
fix-converter-issues
Jun 26, 2026
Merged

fix dflash converter issues#662
rahul-tuli merged 4 commits into
mainfrom
fix-converter-issues

Conversation

@shanjiaz

@shanjiaz shanjiaz commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Fixing two separate issues:

  1. For z-lab style dflash models, older models have block_size on top level but newer models have them nested under dflash_config.

Tests

Tested both locally.

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.

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

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

coderabbitai Bot commented Jun 25, 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: 87cd4676-889a-4aa3-b38d-b16f5a985547

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 updates DFlash conversion to resolve block_size from either checkpoint config layout and changes SpeculatorModel.from_pretrained to accept and forward an explicit verifier argument during external checkpoint conversion.

Changes

Checkpoint conversion and verifier forwarding

Layer / File(s) Summary
DFlash block-size resolution
src/speculators/convert/dflash/converter.py
_build_config reads block_size from either the top-level checkpoint config or dflash_config, raises if neither is present, and uses the resolved value for speculative_tokens and DFlashSpeculatorConfig.block_size.
Explicit verifier forwarding
src/speculators/model.py
SpeculatorModel.from_pretrained adds a verifier parameter and passes it to maybe_convert_external_checkpoint instead of reading it from kwargs.

Possibly related PRs

  • vllm-project/speculators#617: Refines the same DFlash _build_config path that resolves block_size from multiple checkpoint config locations.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately reflects the main dflash converter fixes.
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 matches the changes, covering dflash block_size layout differences and the verifier argument update.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-converter-issues

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.

@mergify

mergify Bot commented Jun 25, 2026

Copy link
Copy Markdown

The quality checks have failed. Please run make style and make quality under
the root directory to address the lint failures. You will need to install the
dev optional install to get the required linting packages:
https://github.com/vllm-project/speculators/blob/main/CONTRIBUTING.md

Signed-off-by: shanjiaz <hezhao@redhat.com>
@mergify mergify Bot removed the quality-failed label Jun 25, 2026

@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.

By making verifier a named param and consuming it (no longer in **kwargs), it's never forwarded to the constructor anymore. So EagleSpeculator gets verifier=None, falls back to the config's name_or_path, and tries to load that instead of using the passed-in object. I believe that would break test_eagle_speculator_from_pretrained_different_layers and ..._local_marshalling_different_layers at runtime (they assert model.verifier == mock_verifier)

Comment thread tests/unit/models/test_eagle_model.py Outdated
shanjiaz and others added 2 commits June 26, 2026 10:11
@shanjiaz
shanjiaz requested a review from rahul-tuli June 26, 2026 14:12

@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.

LGTM; The failing tests seem to be unrelated to this PR 🚢

@rahul-tuli
rahul-tuli enabled auto-merge (squash) June 26, 2026 15:09
@rahul-tuli
rahul-tuli merged commit 7c50e60 into main Jun 26, 2026
9 checks passed
@rahul-tuli
rahul-tuli deleted the fix-converter-issues branch June 26, 2026 15:59
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.

2 participants