fix(peagle): publish the drafter head and embedding P-EAGLE actually trains - #64
Open
khazic wants to merge 1 commit into
Open
Conversation
…trains The hot-publish filter decided what to ship by name and by model_type: it dropped every "embed_tokens.weight" unconditionally, and dropped "lm_head.weight" for any backend whose model_type was not "eagle3". Both rules are wrong for P-EAGLE. Its draft owns an lm_head over the draft vocabulary, and it is the only backend that deliberately does not call freeze_embedding() (speculators trains the draft embedding), so both tensors get a gradient on every drafter step and both were being filtered out. The rollout engine would keep the initial head and the target-seeded embedding for the whole run while training moved on, which is a silent train/serve divergence. model_type is not a usable proxy for this: EAGLE-1/2 deliberately report "eagle3" so they can reuse the data plumbing. Replace the name/model_type heuristic with an explicit per-backend contract (trains_draft_lm_head, trains_draft_embeddings) that each backend declares next to the freeze calls it actually makes. Offline export is unaffected; it goes through _get_full_export_state_dict, which already keeps frozen parameters and buffers. Signed-off-by: khazic <khazzz1c@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The hot-publish filter decides what to ship to the rollout engine by tensor name and by
model_type:Both rules are wrong for P-EAGLE:
lm_headover the draft vocabulary (LlamaForCausalLMPeagle.lm_head), andmodel_typeis"peagle", so the head is filtered out.freeze_embedding().build_modelseeds the embedding from the target and leaves it trainable (speculators setsembed_requires_grad=True), so the embedding is filtered out too.Both tensors receive a gradient on every drafter step. The rollout engine therefore keeps the initial head and the target-seeded embedding for the whole run while training moves on, which is a silent train/serve divergence in the two most consequential tensors of the draft.
model_typeis also not a usable proxy for this question in general:Eagle1TrainerBackend.model_typedeliberately returns"eagle3"so it can reuse the EAGLE-3 data plumbing, so the check is already reading a value that does not mean what it looks like it means.Fix
Replace the name/
model_typeheuristic with an explicit per-backend contract that each backend declares next to the freeze calls it actually makes:trains_draft_lm_headtrains_draft_embeddingsEagle3TrainerBackendTrueFalseEagle1TrainerBackendFalse(weight-tied to the target head)FalsePEagleTrainerBackendTrueTrueDFlashTrainerBackend/DSpark/DominoFalseFalseOffline export is unaffected: it goes through
_get_full_export_state_dict, which already keeps frozen parameters and persistent buffers.Scope
P-EAGLE rollout is not wired up yet (
_speculative_method_from_drafterraises forPEAGLEand points users at training the drafter withdrafter.enable=false), so this is a latent bug rather than a live one today. It becomes a live silent divergence the moment the parallel-drafting runtime lands, which is why it is worth closing now.Validation
Ran a before/after repro on both
mainand this branch. It builds a tiny P-EAGLE draft, runs the real publish filter over its state dict with a realPEagleTrainerBackend, and reports which trained tensors reach the engine.Before/after repro output
Tests
Two new tests in
tests/integration/test_rollout_publish_contract.py:test_publish_state_filter_keeps_peagle_trained_head_and_embeddingcovers the P-EAGLE publish set.test_backend_publish_contract_matches_what_each_backend_trainspins the declared flags on every backend, so a new backend cannot silently inherit the wrong answer.The three existing
test_publish_state_filter_*tests build their backend as aSimpleNamespace, so they now declare the two flags explicitly. That is the point of the change: the stub has to state the contract it is exercising instead of relying on amodel_typestring.Full CPU suite (
tests/integration tests/compat tests/config tests/examples) run on both sides:Test suite before/after
The same 9 tests fail on
mainand on this branch. They need optional dependencies (VeOmni, the NPU vLLM stack) that are absent in this environment, so they are pre-existing and unrelated. The+2on this branch are the new tests above.