Repository navigation
[fix] Kimi-K3.1 DSpark under PP: share the target's embedding on the last stage #3023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f826563
7d3fa50
63e2c61
2026075
cb9f7f9
f65a847
9f3b5be
703592a
b12cbdf
ac819d3
6ca42d1
1e25f53
e4d14af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,17 @@ def make_layers( | |
| return start_layer, end_layer, modules | ||
|
|
||
|
|
||
| def pp_holds_embedding(atom_config) -> bool: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [F2]
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: the last stage builds the embedding only when |
||
| """Whether this PP stage builds the token embedding: the first stage embeds | ||
| the prompt, and the last holds a copy for a drafter that shares it.""" | ||
| from aiter.dist.parallel_state import get_pp_group | ||
|
|
||
| from atom.models.registry import serves_pp_speculation | ||
|
|
||
| pp = get_pp_group() | ||
| return pp.is_first_rank or (pp.is_last_rank and serves_pp_speculation(atom_config)) | ||
|
|
||
|
|
||
| # NOTE: don't use lru_cache here because it can prevent garbage collection | ||
| _model_to_pp_missing_layer_names: dict[int, list[str]] = {} | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F7] Minor: this goes back from a tuple to a mutable
ClassVar[list[str]]shared by all instances. The tuple only existed for the removed per-stage rebinding and other models use a list, so this is fine if the intent is to match the repo convention; just confirming it is deliberate rather than a mechanical revert.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deliberate: the tuple only existed for the per-stage rebinding this PR removes, and a list matches the other drafts.