Repository navigation
Conversation
yafshar
marked this pull request as ready for review
August 31, 2026 21:14
Contributor
Author
|
@hubert-marek @mikasenghaas mind a look? Small one: the ring-CP path was the last place importing Verified on three machines with no flash-attn installed: |
yafshar
force-pushed
the
chore/lazy-ring-flash-attn-imports
branch
2 times, most recently
from
September 7, 2026 21:43
edadafe to
fdc1007
Compare
Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
yafshar
force-pushed
the
chore/lazy-ring-flash-attn-imports
branch
from
September 9, 2026 18:54
fdc1007 to
2ca6f86
Compare
yafshar
force-pushed
the
chore/lazy-ring-flash-attn-imports
branch
from
September 30, 2026 21:19
b3b0f19 to
2ca6f86
Compare
…-attn-imports Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
…sh-attn-imports Signed-off-by: Yaser Afshar <yaser.afshar@intel.com> # Conflicts: # src/prime_rl/utils/cp.py
samsja
pushed a commit
that referenced
this pull request
Oct 19, 2026
…nly CP style (#4134) Removes ring attention context parallelism. Ulysses is now the only CP implementation, so the `cp_style` option is gone. - Deletes `layers/ring_attn.py`, `substitute_ring_attn`, `substitute_gpt_oss_ring_attention`, the ring branches in `utils/cp.py`, and the `ring-flash-attn` dependency plus its `_compat` shim. - Removes `model.cp_style` and `resolve_cp_style`. `setup_context_parallel` always installs the Ulysses attention patch. `setup_cp_params` / `setup_cp_attention_params` lose their `cp_group` and `cp_style` arguments. - DeepSeek-V4 and GLM-5 DSA keep their model-internal CP: queries stay sequence-sharded and keys are all-gathered inside the attention layer. This never used the ring patch. For DeepSeek-V4, the KV all-gather is 1 KB/token (MQA, head_dim 512), and the sparse attention work is already balanced. Ulysses would move 64 heads × 512 of queries and outputs each way, and its kernels need ≥32 heads per rank, so DeepSeek-V4 stays on the all-gather. - `CPSupport` / `cp_support` becomes `cp_blocker(config, cp) -> str | None`. The default rejects a `cp` that cannot shard the attention heads at model setup, for example `cp=3 must divide the 32 attention heads and divide or be a multiple of the 4 KV heads for Ulysses CP`. Before, `cp_style="auto"` fell back to ring in this case. DeepSeek-V4 and GLM-5 DSA override it to `None`. - Removes `cp_style = "ulysses"` from the example/advanced configs. Configs that still set `cp_style` now fail validation. **Breaking:** `trainer.model.cp_style` is removed. Models whose head count is not divisible by `cp` can no longer use CP. Related open PRs: - #4068 changes how ring attention params are passed. This PR deletes that code path. - #3446 defers the ring-flash-attn imports. This PR removes the dependency. - #3724 touches CP padding in `utils/cp.py` / the train loops. Expect conflicts in `setup_cp_params` call sites. - #4057 (DeepSeek-V4.1) overrides `cp_support` with an empty style set. After this PR, that override becomes `cp_blocker` returning its reason. ### Validation SFT, 5 steps, real weights truncated to 4 layers, fake packed variable-length data (seq_len 16384, batch 4). Each run is `main` vs this PR on 4 H200s each, with the same seed. On `main`, `cp_style="auto"` resolved to Ulysses for the FlashAttention models and to ring (the KV all-gather) for DeepSeek-V4. | model / CP | loss main | loss PR | max Δloss main vs PR | max Δloss main vs main | max rel Δ grad norm main vs PR | max rel Δ grad norm main vs main | peak mem main / PR | |---|---|---|---|---|---|---|---| | Qwen3-30B-A3B, cp=2 (Ulysses) | 16.4002 16.2154 16.0340 15.7772 14.8917 | 16.4002 16.2157 16.0356 15.7765 14.8915 | 0.0016 | 0.0018 | 0.84% | 0.99% | 14.6 / 14.6 GiB | | GLM-4.5-Air, cp=4 (Ulysses) | 15.3254 14.8527 13.9303 13.5545 12.9239 | 15.3254 14.8528 13.9303 13.5546 12.9235 | 0.0004 | 0.0001 | 0.20% | 0.17% | 40.6 / 40.6 GiB | | GLM-5.3 DSA, cp=4 (own KV all-gather) | 13.3890 13.3431 13.2668 13.2369 13.1076 | 13.3890 13.3431 13.2667 13.2369 13.1076 | 0.0001 | 0.0001 | 0.03% | 0.01% | 81.3 / 81.3 GiB | | DeepSeek-V4-Flash, cp=2 (own KV all-gather) | 20.2111 19.9461 19.3640 19.2330 18.6930 | 20.2111 19.9463 19.3641 19.2329 18.6930 | 0.0002 | 0.0001 | 0.04% | 0.07% | 124.1 / 124.1 GiB | "main vs main" is a second pair of `main` runs from the same configs and seed. In every model, the main-vs-PR deltas are within or near that run-to-run noise. Step 1 loss is identical in every run. The Qwen3-30B step-1 grad norm ranges 254.31–255.22 across all four of its runs. Tests: - `pytest -m "not gpu"` on tests/unit/orchestrator, tests/unit/test_configs.py, tests/unit/train, tests/unit/utils: all pass except `test_cpu_adam.py::test_native_cpu_adamw_matches_fused_torch_and_preserves_gradients`, which fails on `main` too. - GPU: `test_gpt_oss.py -k context_parallel` (torchrun ×2, Ulysses with sharded sinks), `test_deepseek_v4_kernels.py -k context_parallel` (12 passed), `qwen3_5/test_model.py` (5 passed). - `ruff check` / `ruff format`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **High Risk** > Breaking distributed-training behavior: removes `cp_style` and ring fallback, so invalid `cp` vs head counts now error instead of using ring; large deletion in the CP/attention stack affects all long-context training jobs. > > **Overview** > **Removes ring context parallelism** so Ulysses is the only trainer CP path: deletes `ring_attn.py`, ring attention patching, the `ring-flash-attn` GPU dependency, and related `_compat` shims. > > **Config and API cleanup:** Drops `trainer.model.cp_style` and `resolve_cp_style`; `setup_context_parallel` always installs Ulysses. `CPContext` no longer carries a style; `setup_cp_params` / `setup_cp_attention_params` no longer take `cp_group` or `cp_style`. Example/advanced TOMLs lose explicit `cp_style = "ulysses"`. > > **Validation model change:** `CPSupport` / `cp_support` becomes `cp_blocker(config, cp) -> str | None` on model classes—the default enforces Ulysses head-sharding rules at setup (configs that previously relied on `cp_style="auto"` falling back to ring will now fail). DeepSeek-V4 and GLM-5 DSA override with `None` because they keep **model-internal** CP (sequence-sharded queries, all-gathered keys), unchanged by this refactor. > > Docs in `scaling.md` are updated to describe Ulysses-only CP and sparse-attention all-gather behavior instead of ring vs Ulysses. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit c167f05. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Contributor
Author
|
Superseded by #4134, which removes ring CP and the ring-flash-attn dependency. |
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.
Change
ring_flash_attnimportsflash_attnat module scope, so importing the trainer requires a flash-attn build even for runs that never touch ring attention: no-CP runs (cp=1, the default) andcp_style="ulysses"runs.After #3533 unified context-parallel setup, the imports that eagerly load it from
src/prime_rl/utils/cp.pywere consolidated into one module-scope line. Move both names into thecp_style == "ring"branches that use them:substitute_hf_flash_attn, called only in the ring branch ofsetup_context_parallelupdate_ring_flash_attn_params, called only in the ring branch ofsetup_cp_attention_paramssrc/prime_rl/trainer/models/layers/ring_attn.pyalso importsring_flash_attnat module scope, butsubstitute_ring_attnalready imports that module lazily, so it is reached only on the ring path.This matches what the surrounding code already does.
flash_attnitself is guarded bytry/except ImportErrorinsrc/prime_rl/trainer/models/layers/attn.py, the ulysses branches incp.pyalready defer their own imports, andsrc/prime_rl/entrypoints/trainer.pydocuments deferring heavy ML imports (ring_flash_attnamong them) as the intent. The ring path was the one place that still imported eagerly.Note this is about the ring path, not the attention backend:
substitute_ring_attnimportsring_flash_attnwhichever flash-attn backendattnselects, socp_style="ring"still needs it regardless ofattn.No behavior change for ring-CP runs with their dependencies installed: the same imports happen, just on first use of the ring path instead of at module import. With the dependencies missing, the error now surfaces when the ring path is first used rather than when the trainer is imported.
prime_rl._compatstays at module scope insrc/prime_rl/utils/cp.py. It patches transformers for bothring_flash_attnand the mamba hub-kernels shim, and it must still run before the deferred imports.Verification
Imported
prime_rl.utils.cpbefore and after, on three machines with neitherflash-attnnorring-flash-attninstalled (torch 2.13.0+cu130 on an L40S and a B200, torch 2.13.0+xpu on an Intel Arc Pro B60). Identical results on all three:The third line is the point: the import moved rather than disappeared, so a ring-CP run still gets a clear error if required ring dependencies are missing.
Module-scope imports reachable from
trainer.rl.train,trainer.sft.trainandtrainer.models, excluding those insidetry/except ImportError:utils/cp.py:12The three guarded
flash_attnimports insrc/prime_rl/trainer/models/layers/attn.pyare untouched by this PR; they already degrade toNonewhen flash-attn is absent.