Apply the context-parallel hooks on the sharded-load path too - #2
Open
whn09 wants to merge 1 commit into
Open
Conversation
`from_pretrained(..., parallel_config=...)` shards weights while reading the checkpoint, so `enable_parallelism` cannot be called afterwards -- it raises by design -- and the loader applies the parallelism itself. It applies tensor parallelism but never the context-parallel hooks, so a `ParallelConfig` that carries both is silently reduced to tensor parallelism alone: no error, correct numbers, and every rank redundantly computing the whole sequence. The context-parallel half of `enable_parallelism` moves into `_apply_context_parallel`, split out for the same reason `_resolve_parallel_config` was, and both sharded-load paths (safetensors and DCP) call it before applying tensor parallelism -- the same order `enable_parallelism` uses. This matters for models that need both: MiniMax-H3 has 56 attention heads, so `tp_degree` caps at 8, and at 33B parameters the `enable_parallelism` route is not an option because every rank would first have to hold the full checkpoint. The sharded-load path is the only way such a model can reach beyond 8 accelerators. Reachable once `ParallelConfig` accepts both configs (huggingface#14725). Tests: a Neuron `torchrun` worker following the `_neuron_tp_worker.py` convention, run at tp_degree=2 x ulysses_degree=4 on a trn2.48xlarge. It asserts that the hooks are registered and that the attention processors received the config -- neither of which output values can detect, since a model that skips them still returns the right answer -- and that the output still matches a single-device reference read back from the same checkpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
10 tasks
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.
Hi @JingyaHuang — a small follow-up on top of your huggingface#14609, targeting your branch rather than
mainbecause the two anchors only exist here. Would you review, and merge it into your branch if you agree with the shape?The problem
from_pretrained(..., parallel_config=...)shards weights while reading the checkpoint, soenable_parallelismcannot be called afterwards — it raises by design, and the message says exactly why. The loader therefore applies the parallelism itself:Tensor parallelism, yes. The context-parallel hooks, no. So a
ParallelConfigcarrying both configs is silently reduced to tensor parallelism alone on this path: no error, no warning, correct output numbers, and every rank redundantly computing the whole sequence. The same holds in_load_dcp_checkpoint.The change
The context-parallel half of
enable_parallelismmoves into_apply_context_parallel(config, cp_plan=None)— split out for the same reason you split out_resolve_parallel_config, and the docstring says so — and both sharded-load paths call it before applying tensor parallelism, the orderenable_parallelismuses.enable_parallelismitself is now just: resolve, CP, TP.This is reachable only once
ParallelConfigaccepts both configs, which is my huggingface#14725. That PR relaxes__post_init__, splits the shared mesh between the two configs, and fixes the Neuron backend's shard index; this one is the loader-side piece it cannot reach.Why it matters, concretely: MiniMax-H3 has 56 attention heads, so
tp_degreecaps at 8, and on a 64-coretrn2.48xlargetensor parallelism alone leaves 56 cores idle. At 33B parameters theenable_parallelismroute is not an option — every rank would first have to hold the full checkpoint in host memory — so your streaming loader is the only path such a model can take, and without this change it cannot use context parallelism at all. With both PRs, H3 goes from 9.285 s/step on 8 cores to 3.941 s/step on 32 (TP=8 x ulysses=4), a 2.36x speedup.How to test
I ran this on a
trn2.48xlarge. The test is in the PR:Result on my box:
Two things worth knowing about how the test is built:
Output values cannot detect this bug. A model that skips the CP hooks still returns the right answer — it just does the work redundantly on every rank. So the worker asserts on the structure: that
cp_input---*/cp_output---*hooks are registered, and that the attention processors received theParallelConfig(without which attention runs with no Ulysses all-to-all). It then also compares against a single-device reference read back from the same checkpoint, to catch anything the hooks might break.Negative control. With the one call removed and everything else identical, the worker exits 1 on exactly the intended assertion:
Rank 0 writes the checkpoint the other ranks read, so it is single-node, like the other Neuron workers.
Caveats
"neuron"distributed backend andtorchrun, following the_neuron_tp_worker.pyconvention already in your branch). Porting it to NCCL is essentially the backend string and the device selection — happy to add a CUDA counterpart if you'd rather have one that CI can run._load_dcp_checkpointcall site is the same one-liner in the same position, but I have not run it; if you have a DCP checkpoint handy that would be a useful second check.make_neuron_sharded_load_specraisesnum_attention_headsto 8 so the head count survives being divided twice (tp_degree=2leaves 4 per rank,ulysses_degree=4splits those into 1 each;ulysses_degree=2is not available on Neuron, whose all-to-all only accepts group sizes of 4, 8, 16 or multiples of 32). Allow tensor parallelism and context parallelism in one ParallelConfig huggingface/diffusers#14725 adds amake_neuron_hybrid_specthat does the same thing — whichever lands second should reuse the other's.Separately, the non-persistent-buffer issue I left as a comment on huggingface#14609 is on this same loading path; that one is independent of this PR.