Add TPU USP context parallelism - #4836
Conversation
|
🤖 Hi @huytransformer, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for USP (Ulysses-over-ring) context parallelism on the TPU Tokamax Splash path for training in MaxText. It adds configuration options, layout validation helpers, and integrates the hybrid strategy into the attention operation, accompanied by extensive unit and collective tests. The review feedback focuses on improving robustness by adding defensive null checks in usp_attention.py to prevent potential TypeError exceptions on unsharded tensors, and refactoring device platform checks in attention_op.py to use a more idiomatic numpy flat indexing approach.
There was a problem hiding this comment.
This pull request introduces robust support for TPU USP (Ulysses-over-Ring) context parallelism, allowing MaxText to scale hybrid context parallelism efficiently by combining ring-attention sequence rotation with Ulysses head-exchange all-to-alls. The overall design is exceptionally high quality, extremely clean, highly idiomatic, and very well integrated with the existing MaxText config validation and Tokamax splash-attention paths.
🔍 General Feedback
- Exceptional Testing Quality: The newly added unit tests (
usp_attention_test.pyandusp_collective_test.py) are incredibly thorough. Specifically, forcing an 8-device CPU mesh in a standalone subprocess to verify multi-dimensional collectives, attention parity, and gradient correctness without physical TPU hardware is a masterclass in robust JAX testing. - Robust Layout and Runtime Checks: The USP-specific configurations and constraints are systematically validated at both startup (config types validation) and layer initialization (attention operator layout checks), protecting against unsupported setups.
- Seamless Integration: The extension of logical axis rules in
base.ymland physical axes configuration inmaxtext_utils.pyis elegant, maintaining full backward compatibility with non-USP paths.
| dense_grads = jax.grad(dense_loss, argnums=(0, 1, 2))(query, key, value) | ||
| usp_grads = jax.grad(usp_loss, argnums=(0, 1, 2))(query, key, value) | ||
| for name, dense_grad, usp_grad in zip(("dQ", "dK", "dV"), dense_grads, usp_grads): | ||
| np.testing.assert_allclose(jax.device_get(usp_grad), jax.device_get(dense_grad), atol=1e-5, err_msg=name) |
There was a problem hiding this comment.
🟢 Low - Adding rtol=1e-5 to gradient comparison as well ensures numerical stability across different architectures.
| np.testing.assert_allclose(jax.device_get(usp_grad), jax.device_get(dense_grad), atol=1e-5, err_msg=name) | |
| np.testing.assert_allclose(jax.device_get(usp_grad), jax.device_get(dense_grad), rtol=1e-5, atol=1e-5, err_msg=name) | |
| ```</COMMENT> |
| raise ValueError( | ||
| "TPU USP attention requires max_target_length to be divisible by ici_context_parallelism squared." | ||
| ) | ||
| if self.num_query_heads % usp_ulysses_size != 0: |
There was a problem hiding this comment.
🟡 Medium - Checking that self.num_query_heads is divisible by usp_ulysses_size is a great static sanity check. Note that if tensor model parallelism is enabled, heads are first sharded across tensor_parallelism. The actual constraint is that the local query head count (after tensor sharding) must be divisible by Ulysses size. While validate_head_sharding dynamically catches this during execution, we could consider documenting this interaction or eventually validating it here statically.
There was a problem hiding this comment.
wow go gemini
There was a problem hiding this comment.
you could add in the TP factor as well but generally no one is using TP anyway. This is just a config check, if a user is setting TP and going over head count they will hit a less user friendly jax sharding error
There was a problem hiding this comment.
fwiw I think the TP case doesn't reach jax sharding error. validate_head_sharding already checks the local head at init with test
maxtext/src/maxtext/kernels/attention/ulysses_attention.py
Lines 186 to 196 in bdafbc9
|
|
||
| dense_output = _dense_reference_attention(query, key, value, segment_ids) | ||
| usp_output = usp_attention_fn(query, key, value, segment_ids) | ||
| np.testing.assert_allclose(jax.device_get(usp_output), jax.device_get(dense_output), atol=1e-5) |
There was a problem hiding this comment.
🟢 Low - Adding a relative tolerance (rtol) to assert_allclose is recommended to prevent potential flakiness under different CPU architectures or compiler versions, especially when comparing standard dot-product attention with block/gathered attention.
| np.testing.assert_allclose(jax.device_get(usp_output), jax.device_get(dense_output), atol=1e-5) | |
| np.testing.assert_allclose(jax.device_get(usp_output), jax.device_get(dense_output), rtol=1e-5, atol=1e-5) | |
| ```</COMMENT> |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
f9a0d97 to
293acad
Compare
27a6b5f to
b9eea0d
Compare
| "TPU Ulysses attention requires num_kv_heads " | ||
| f"({self.num_kv_heads}) to be divisible by context_parallel_size ({context_parallel_size})." | ||
| ) | ||
| if context_parallel_strategy != "usp" and ( |
There was a problem hiding this comment.
Isn't context_parallel_strategy=ulysses stil supported? E.g. only ulysses, no ring?
There was a problem hiding this comment.
If a user wanted to run only ulysses without any ring, what would they set? It is not clear to me
There was a problem hiding this comment.
so for every CP we set ici_context_parallelism, only usp additionally sets the ulysses else rejected at init.
| raise ValueError( | ||
| "ici/dcn_context_ulysses_parallelism was specified, but is only supported when " | ||
| "context_parallel_strategy='usp'." | ||
| ) |
There was a problem hiding this comment.
there is a lot of logic here - can you wrap all of this into a function (style suggestion go/small-functions)
| raise NotImplementedError("TPU USP attention does not support record_max_logits yet.") | ||
|
|
||
|
|
||
| def with_sequence_axes(axis_names: Any, ring_axis: str, ulysses_axis: str, sequence_dim: int) -> Any: |
There was a problem hiding this comment.
renmae to shard_by_ring_and_ulysses or shard_by_usp or similar?
There was a problem hiding this comment.
renamed to with_usp_sequence_axes!
| attention_output = ulysses_attention.inverse_ulysses_all_to_all(attention_output, context_axis) | ||
| return attention_output, None | ||
|
|
||
| if use_usp: |
There was a problem hiding this comment.
I would also wrap everything under the if in a function following style go/small-functions
b9eea0d to
bdafbc9
Compare
| if usp_ring_size <= 1: | ||
| raise ValueError("TPU USP attention requires ici_context_parallelism > 1 for the ring dimension.") | ||
| if usp_ulysses_size <= 1: | ||
| raise ValueError("TPU USP attention requires ici_context_ulysses_parallelism > 1 for the Ulysses dimension.") |
There was a problem hiding this comment.
what if ici_context_parallelism=-1 or ici_context_ulyses_parallelism=-1
| if self.context_sharding not in ("context", "expert"): | ||
| raise ValueError(f"Assigned context_sharding f{self.context_sharding} is not supported.") | ||
| if self.ulysses_context_sharding != "context_ulysses": | ||
| raise ValueError(f"Assigned ulysses_context_sharding {self.ulysses_context_sharding} is not supported.") |
There was a problem hiding this comment.
we will remove this limit in the future using component sharding!!
| axis_names_kv: Any, | ||
| dkv_dim_q: int, | ||
| dkv_dim_kv: int, | ||
| attention_label: str, |
There was a problem hiding this comment.
help me understand when we wanna use ulysses context, should we use
ici_context_parallelism=CP
context_parallelism_strategy=ulysses
OR
ici_ulysses_context_parallelism=CP
| shard_mode: "auto" # can be either auto or explicit | ||
| custom_mesh_and_rule: "" # replace default mesh and logical rule by specifying yml name under config/mesh_and_rule/. | ||
| mesh_axes: ['diloco', 'data', 'stage', 'fsdp', 'fsdp_transpose', 'context', 'context_autoregressive', 'tensor', 'tensor_sequence', 'expert', 'autoregressive'] | ||
| mesh_axes: ['diloco', 'data', 'stage', 'fsdp', 'fsdp_transpose', 'context', 'context_ulysses', 'context_autoregressive', 'tensor', 'tensor_sequence', 'expert', 'autoregressive'] |
There was a problem hiding this comment.
please update mesh_axes in types.py as well
Description
This PR introduces
context_parallel_strategy=usp(USP, Ulysses over ring). Follow up to #4687.Currently does not support load balancing + sequence packing.
Tests
python3 -m pytest tests/unit/configs_value_test.py tests/unit/usp_attention_test.py tests/unit/usp_collective_test.pyPassed.
python3 -m pytest tests/unit/attention_test.py -k usp3 passed.
Performance
llama3-8b, v5p (64 chips), CP64, bf16, synthetic data, global batch 1, no load-balancing
Reporting median step time (s)
Example repro command:
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.