Add shared resolve_target_layer_ids util for eagle3 and dflash#457
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:
WalkthroughThis change standardizes how Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
608b0fc to
aa6c8fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/speculators/models/dflash/core.py`:
- Around line 64-69: The constructor DFlashDraftModel.__init__ currently
requires config.aux_hidden_state_layer_ids to be non-None (raising if None), but
the config schema still defaults that field to None which breaks
from_pretrained() loads; either make aux_hidden_state_layer_ids required at the
config schema level in DFlashSpeculatorConfig (remove the None default) or,
preferably, add a fallback in DFlashDraftModel.__init__ to call the existing
resolve_target_layer_ids() (the same logic used by from_training_args()) when
config.aux_hidden_state_layer_ids is None so models loaded via from_pretrained()
get resolved target_layer_ids instead of raising. Ensure you reference and
update DFlashDraftModel.__init__, DFlashSpeculatorConfig,
resolve_target_layer_ids(), and from_training_args()/from_pretrained()
accordingly.
🪄 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: 24b5c8f5-0952-4f0a-a14b-209d5574fa93
📒 Files selected for processing (3)
src/speculators/models/dflash/core.pysrc/speculators/models/eagle3/core.pysrc/speculators/models/utils.py
aa6c8fa to
2117567
Compare
Signed-off-by: Fynn Schmitt-Ulms <fschmitt@redhat.com>
2117567 to
fa52420
Compare
shanjiaz
left a comment
There was a problem hiding this comment.
lol thanks for refactoring this.
<!-- markdownlint-disable --> PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED. ## Purpose Closes #454 Combines logic for target layer id resolution in both eagle3 and dflash. <!--- Why your changes are needed --> ## Description Creates a shared util fn `resolve_target_layer_ids` which both algorithms call in their `from_training_args` function. <!--- High-level concise summary of changes --> ## Related Issue <!--- Link related issue if applicable --> #454 ## Tests I locally ran `tests/e2e/smoke/test_online_training.py::test_online_smoke` which succeeded. <!--- Please describe in detail how you tested your changes. --> 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: Fynn Schmitt-Ulms <fschmitt@redhat.com>
…m-project#457) <!-- markdownlint-disable --> PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED. ## Purpose Closes vllm-project#454 Combines logic for target layer id resolution in both eagle3 and dflash. <!--- Why your changes are needed --> ## Description Creates a shared util fn `resolve_target_layer_ids` which both algorithms call in their `from_training_args` function. <!--- High-level concise summary of changes --> ## Related Issue <!--- Link related issue if applicable --> vllm-project#454 ## Tests I locally ran `tests/e2e/smoke/test_online_training.py::test_online_smoke` which succeeded. <!--- Please describe in detail how you tested your changes. --> 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: Fynn Schmitt-Ulms <fschmitt@redhat.com>
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
Closes #454
Combines logic for target layer id resolution in both eagle3 and dflash.
Description
Creates a shared util fn
resolve_target_layer_idswhich both algorithms call in theirfrom_training_argsfunction.Related Issue
#454
Tests
I locally ran
tests/e2e/smoke/test_online_training.py::test_online_smokewhich succeeded.I have filled in: