Conversation
load_eagle_model() builds the draft with get_model() but never passes load_config, so get_model() falls back to the target's and SpeculativeConfig.draft_load_config is silently ignored. V1 passes it in LLMBaseProposer._get_model(), and V2's own gemma4 speculator passes it, so the eagle path is the outlier. Forward speculative_config.draft_load_config. It defaults to None, which get_model() already resolves to the target's load config, so behaviour is unchanged when it is unset. Signed-off-by: Stefano Castagnetta <scastagnetta@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 SummarySummary by CodeRabbit
Walkthrough
ChangesDraft model load configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Draft models now honor their configured load settings while unset settings retain existing target-config fallback behavior. The covered forwarding paths present no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Correct propagation boundary. get_model() already defines the None -> target load_config fallback, so forwarding draft_load_config here restores the explicit override without changing default behaviour. The sentinel/None pair pins both sides of the contract.
load_eagle_model reads speculative_config.attention_backend once this branch picks up main, so the stub needs the field to keep working. Signed-off-by: Stefano Castagnetta <scastagnetta@nvidia.com>
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed current ceb4338. The latest change only extends the draft load-config test fixture with attention_backend; the core handoff still passes the draft-specific load_config through to the loader, while None correctly defers to the target/default resolution. No blocker from me on this head.
|
Hi @njhill, this should be the last one in the series :) |
Purpose
load_eagle_model()builds the draft withget_model()but never passesload_config:get_model()resolves that toload_config or vllm_config.load_config, so the draft silently inherits the target's load config andSpeculativeConfig.draft_load_config("Load config for the draft model. If not specified, will use the load config from the target model",vllm/config/speculative.py) has no effect on Model Runner V2.V1 passes it in
LLMBaseProposer._get_model(), and V2's own Gemma4 speculator passes it invllm/v1/worker/gpu/spec_decode/gemma4/speculator.py, so the eagle path is the outlier rather than a deliberate choice.Fix
Forward
speculative_config.draft_load_config. It defaults toNone, whichget_model()already resolves to the target's load config, so nothing changes when it is unset.Validation
Run inside
vllm/vllm-openai:nightly-8a728663c1(0.28.1rc1.dev388+g8a728663c), unpatched then patched against the same installed package. Without the fix the first test fails with the bug's signature:The second test pins the unset case so the fallback to the target's load config stays intact.
Why this is not a duplicate
I ran the checks in AGENTS.md. Nothing open touches
draft_load_configon the V2 path. #54788 and #54826 also editload_eagle_modelformoe_backendandattention_backendrespectively; this is a third, independent field and the three will need a trivial rebase against each other depending on merge order.Model evaluation
Not applicable. The draft's load config selects how its weights are read (load format, download dir), not what they are. With
draft_load_configunset, which is the default, behaviour is byte-identical, and that case is pinned by the second test.AI assistance
AI assistance was used to author this change. I have reviewed every changed line, ran the tests and linters above myself, and can defend the design.