Skip to content

Fix Wrong SP4 Images From NCCL Graph Registration - #915

Merged
NSagan271 merged 1 commit into
mainfrom
nccl-graph-register
Oct 27, 2026
Merged

NSagan271 merged 1 commit into
mainfrom
nccl-graph-register

Conversation

@Gaurav-Shah05

Copy link
Copy Markdown
Collaborator

This PR NCCL_GRAPH_REGISTER=0 by default. The problem was that with registration on, Cosmos3 at SP4 with batched captures returns corrupted images (#897).

Testing

On H100s with expandable_segments:True, GPU pairs swapped between boots:

main This PR
Cosmos3 SP2 t2i, 320x192 0.77 s 0.75 s 1.03x
Cosmos3 SP2 t2i, 832x480 1.13 s 1.16 s 0.98x
Qwen3.5-9B TP2, c32 3,159 tok/s 3,252 tok/s 1.03x

All within run-to-run noise, and the Cosmos3 images match main.

With NCCL registering the all-to-all buffers inside CUDA graphs, Cosmos3 SP4 with batched captures gave wrong images, on #897's exchange and on its first version once its buffers moved; with registration off every variant matched main.
@mstar-review

mstar-review Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

An earlier review run didn't complete; a later run finished: #915 (comment)

@NSagan271

Copy link
Copy Markdown
Collaborator

/review

@NSagan271 NSagan271 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, waiting for the bot review to see if there's anything non-obvious

@mstar-review

mstar-review Bot commented Oct 27, 2026

Copy link
Copy Markdown

Automated review

🟡 No findings on 59a2abf, light review

Description

  • The Testing table covers Cosmos3 SP2 and Qwen3.5-9B TP2 but not Cosmos3 SP4 with batched captures, which is the configuration the PR exists to fix, so nothing in the description shows SP4 images coming out correct with this change.
  • "With it off, everything matches main exactly" and "the Cosmos3 images match main" don't say what was compared (bit-exact pixels or by eye), on which prompts or seeds, or at which SP degree.
  • The mechanism bullets ("the output depends on where the exchange buffers sit in memory", "Sequence parallel: one captured all-to-all for Ulysses; drop the all-gather exchange #897's first version breaks too once its buffers move") are stated as fact with no evidence such as a minimal repro, NCCL debug output or a test that fails without the change; AGENTS.md asks for an unverified mechanism to be labeled a hypothesis.
  • Missing: the torch, CUDA and NCCL versions used, the command or config that reproduces the SP4 corruption, and which CPU tests were run (for example test/modular/test_platforms.py) and their results.

Questions

  • Is the corruption understood as an NCCL/PyTorch problem (for example, expandable segments remapping graph-pool memory after NCCL has registered it, possibly fixed in a known NCCL version), or could turning off registration be hiding an M* buffer-lifetime bug in the SP exchange path? If it's the latter, the bug could still corrupt output on other paths and this change would only be masking it.
What the review checked
  • apply_process_env_defaults uses os.environ.setdefault and is called at the top of _worker_process_target and in Conductor._launch_workers before spawn, so NCCL_GRAPH_REGISTER=0 is in place before NCCL initializes, and a value the user sets still wins.
  • _built_for() picks CudaPlatform from torch.version.cuda without a device call, and ROCm builds return RocmPlatform first, so the default applies only to NVIDIA builds and RCCL is unaffected.
  • No shipped config enables expandable_segments (it appears only in benchmark/cosmos3/reproduce*.sh and test/wan22/launch_server_wan22.sh), which fits the claim that default runs are unchanged.
  • The new env var has a row in docs/environment_variables.rst next to the other NCCL variables, as invariant 9 requires.
  • Existing test/modular/test_platforms.py tests monkeypatch process_env_defaults, so the new CUDA default doesn't break them.

Advisory, not a merge gate. Rules: AGENTS.md. Reply to discuss.

@NSagan271
NSagan271 merged commit ed995d9 into main Oct 27, 2026
9 of 10 checks passed
@NSagan271
NSagan271 deleted the nccl-graph-register branch October 27, 2026 21:37
dghosef added a commit that referenced this pull request Oct 28, 2026
Main brought (since 6cfe39c): #892 Parakeet and Nemotron ASR with the
transducer as a graph loop (and the shared mrope_position_ids helper),
#917 pytest 9.2 flag proxies, #897 one captured all-to-all for Ulysses,
#915 NCCL graph registration off by default, #836 Qwen3-Omni Thinker
prefix caching (ARNodeSubmodule.cuts_tensor_inputs, marks_dynamic_dims,
the conductor's _streamed_partitions), #909 bench cells, #900 Command A+
batched image tiles.

Conflict:
- mstar/model/submodule_base.py: main's ARNodeSubmodule.cuts_tensor_inputs
  beside the stack's token_stop and its stop checks. Independent class
  attributes; both kept.

Ported onto the stack's mechanisms:
- mstar/model/parakeet/submodules.py: check_stop_batched drops the
  step_walk parameter (the stack's three-argument signature) and reads
  its "active" flags through HostRows.first_values instead of its own
  row-map loop. Its stop is a flag, not a token, so it has no TokenStop.
- test/modular/test_transducer_loop.py: the call without step_walk.

The conductor's _streamed_partitions (#836) sits beside _pick_replicas /
_register_request; it reads only partition_states and _resource_specs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dghosef added a commit that referenced this pull request Oct 28, 2026
Brings #858's merge of main: #892 Parakeet / Nemotron ASR, #917, #897
Ulysses all-to-all, #915, #836 Qwen3-Omni Thinker prefix caching, #909,
#900.

Conflicts:
- docs/adding_models.rst: main's marks_dynamic_dims sentence and #859's
  compile_regions paragraph are independent facts; both kept, in one
  paragraph.
- mstar/model/qwen3_omni/submodules.py (3 hunks, plus auto-merged hunks
  in the same class): AudioEncoderSubmodule. Two mechanisms for one cause
  (the HF encoder recompiling per clip length): #859 runs it eagerly
  (disable_torch_compile) and packs a step's clips into calls of at most
  MAX_BATCH_FRAMES; #836 builds the encoder's chunk tensors in
  prepare_inputs with mark_dynamic so the per-clip forward compiles once
  (marks_dynamic_dims, _max_seqlen). One mechanism: #859's class verbatim.
  Under disable_torch_compile the engine never compiles the encoder, so
  #836's concern (a recompiled graph rounding differently from the one
  whose output the prefix cache holds) cannot arise. The auto-merge had
  taken #836's kwargs-only forward beside #859's forward_batched, which
  calls forward with the clips positionally; that keep-both is undone.
  Main's VisionEncoderSubmodule (marks_dynamic_dims, prepared indices) and
  Thinker changes (cuts_tensor_inputs, split_inputs, mrope_position_ids,
  image_span) are kept; #859 did not change those. Imports: both
  cp_embed_accumulate and mrope_position_ids; the audio helpers only
  #836's prepare_inputs used are dropped.

Tests following from it (test/modular/test_qwen3_omni_prefix_cache.py):
- the test that #836's prepared chunk tensors embed a clip as the encoder
  does alone is deleted with that code;
- the compile test now asserts the audio encoder stays eager while its
  output is cached;
- the CUDA no-recompile test keeps its vision case only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dghosef added a commit to dghosef/mstar that referenced this pull request Oct 28, 2026
…igin/main 73c6c17) into merge/860-main

Brings main's mstar-project#892 Parakeet / Nemotron ASR, mstar-project#917, mstar-project#897 Ulysses as one
captured all-to-all (prefer_all_gather and the all-gather exchange gone,
Cosmos3 captures every tier under TP x SP), mstar-project#915, mstar-project#836 Qwen3-Omni Thinker
prefix caching, mstar-project#909 and mstar-project#900, with mstar-project#858's and mstar-project#859's resolutions.

No conflicts. Files both sides touched (docs/clients.rst,
docs/installation.rst, docs/models.rst, sequence_parallel.py,
cosmos3/submodules.py, test_accelerator_graph_capture.py,
test_flux2_klein_model.py): each merged change equals mstar-project#859's own delta
since 3166401, so nothing kept both sides. mstar-project#860's my_rows / gather_rows
(Wan2.2's CFG split) sit beside main's new ulysses_attention and use
CommGroup.all_gather, which mstar-project#897 left unchanged; nothing in mstar-project#860 passed
prefer_all_gather.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants