Add DFlash model converter - #617
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 ChangesDFlash Conversion Support
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/convert/test_dflash_converter.py (1)
33-75: 🏗️ Heavy liftExpand coverage to
_save,_validate, and entrypoint dispatch.These tests only exercise
_build_config; the new conversion logic in_save/_validateandconvert_model(..., algorithm="dflash")remains untested. Add focused tests for missing critical keys, NaN validation failure, and DFlash branch wiring.As per coding guidelines,
tests/**/*.py: "Verify that new code paths introduced in the PR are covered."🤖 Prompt for 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. In `@tests/unit/convert/test_dflash_converter.py` around lines 33 - 75, The test file currently only covers the _build_config method in the TestBuildConfig class, leaving critical code paths untested. Add new test methods to cover the _save method (including scenarios with missing critical keys), the _validate method (including NaN validation failure cases), and the entrypoint dispatch logic in convert_model when algorithm="dflash" is specified. Create separate test methods within TestBuildConfig or new test classes (TestSave, TestValidate, TestEntrypoint) that mock dependencies appropriately and verify the correct behavior and error handling for each of these three areas.Source: Coding guidelines
🤖 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/dflash/converter.py`:
- Line 150: The code at line 150 in the converter.py file uses
next(iter(body.values())) to infer the dtype without checking if the body
dictionary is empty. If all keys are filtered out or the checkpoint is
malformed, this will raise a StopIteration exception which is hard to diagnose.
Add a guard condition to check if body is not empty before attempting to infer
the dtype from body.values(), and either skip the dtype conversion or raise a
more informative error message if body is empty.
- Around line 133-143: The critical_missing validation currently only checks for
missing keys that start with "layers.", but other required draft-body weights
like "fc.weight", "hidden_norm.*", and "norm.*" can also be absent and will
silently initialize with default values instead of checkpoint values. Expand the
critical_missing filter to include all required weight patterns in the
DFlashDraftModel, not just "layers.*", by adding additional conditions to check
for missing keys from the "fc", "hidden_norm", and "norm" components, ensuring
that any absent critical weights are caught and raised as an error.
---
Nitpick comments:
In `@tests/unit/convert/test_dflash_converter.py`:
- Around line 33-75: The test file currently only covers the _build_config
method in the TestBuildConfig class, leaving critical code paths untested. Add
new test methods to cover the _save method (including scenarios with missing
critical keys), the _validate method (including NaN validation failure cases),
and the entrypoint dispatch logic in convert_model when algorithm="dflash" is
specified. Create separate test methods within TestBuildConfig or new test
classes (TestSave, TestValidate, TestEntrypoint) that mock dependencies
appropriately and verify the correct behavior and error handling for each of
these three areas.
🪄 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: 0ab64bec-032a-403a-9221-c3a2aee8d1c2
📒 Files selected for processing (4)
src/speculators/convert/dflash/__init__.pysrc/speculators/convert/dflash/converter.pysrc/speculators/convert/entrypoints.pytests/unit/convert/test_dflash_converter.py
|
The quality checks have failed. Please run |
60dff12 to
3548e66
Compare
0a8f412 to
c263a7c
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
c263a7c to
3496fd2
Compare
|
Thanks @shanjiaz ! |
<!-- markdownlint-disable --> <!-- PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED. --> ## Purpose Close vllm-project#616 External DFlash checkpoints (e.g. z-lab/*-DFlash) had no path into the Speculators format, so they could not be loaded or finetuned via from_pretrained. ## Tests - add mocked _build_config unit tests - test on a real checkpoint: converted `z-lab/Qwen3-8B-DFlash-b16` with verifier `Qwen/Qwen3-8B`, reloaded via `from_pretrained` with weights bit-identical to source ## 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: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com> Co-authored-by: shanjiaz <zsjwpianpian@gmail.com>
<!-- 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>
Purpose
Close #616
External DFlash checkpoints (e.g. z-lab/*-DFlash) had no path into the Speculators format, so they could not be loaded or finetuned via from_pretrained.
Tests
z-lab/Qwen3-8B-DFlash-b16with verifierQwen/Qwen3-8B, reloaded viafrom_pretrainedwith weights bit-identical to sourceChecklist
I have filled in: