Skip to content

refactor(cp): sharder ownership - #3187

Merged
akoumpa merged 6 commits into
huiyingl/refactor/cp-unifyfrom
akoumpa/refactor/cp-sharder-ownership
Jul 22, 2026
Merged

refactor(cp): sharder ownership#3187
akoumpa merged 6 commits into
huiyingl/refactor/cp-unifyfrom
akoumpa/refactor/cp-sharder-ownership

Conversation

@akoumpa

@akoumpa akoumpa commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Simplifies the context-parallel forward API introduced by #2937 and makes ContextParallelSharder the single recipe-facing owner of CP batch preparation and token layout.

The sharder now:

  • derives TE and Magi behavior from the live model instead of accepting use_te, magi, or backend arguments;
  • derives packed THD input from batch["qkv_format"];
  • reuses a model-owned prepare_model_inputs_for_cp sharder when one is available;
  • binds the CP/TP meshes once, so token shard/gather calls do not accept a mesh argument;
  • shards the batch through shard(batch) and returns (train_ctx, sharded_batch);
  • lives under nemo_automodel.components.distributed.context_parallel, with only ContextParallelSharder exported at the package level.

API

from nemo_automodel.components.distributed.context_parallel import ContextParallelSharder

cp_sharder = ContextParallelSharder(
    model,
    device_mesh,
    batch,
    padding_token_id=tokenizer.pad_token_id,
)
train_ctx, batch = cp_sharder.shard(batch)

# Optional token-aligned tensors use the same captured CP layout.
local_advantages = cp_sharder.shard_token_tensor(
    advantages,
    fill=0.0,
)

with train_ctx():
    outputs = model(**batch)
    local_logprobs = compute_token_logprobs(outputs, batch["labels"])

full_logprobs = cp_sharder.gather_token_tensor(
    local_logprobs,
    trim=True,
)

There is intentionally no separate .context() call. shard(batch) performs the batch transformation, captures the resulting token layout, and returns the context factory required by the model forward.

shard_token_tensor(...) and gather_token_tensor(...) use the mesh and layout already bound to the sharder. This keeps auxiliary token tensors aligned with round-robin SDPA, contiguous model-owned layouts, packed THD partitions, and Magi dispatch permutations without adding them to the model batch.

A DTensor is not returned because standard Shard(dim) placements cannot represent all supported CP layouts, including head/tail ordering, packed THD partitions, Magi permutations, and model-owned re-padding.

Changelog

  • Remove the recipe-facing prepare_cp_forward wrapper.
  • Rename ContextParallelismSharder to ContextParallelSharder.
  • Move CP implementation modules under components/distributed/context_parallel/.
  • Update LLM, VLM, dLLM, KD, model-owned CP hooks, tests, and tools to use the new package and API.
  • Keep backend-specific and model-specific preparation behind the sharder boundary.

Validation

  • 138 passed across the focused CP sharder, CP utility, differential coverage, input-embedding, and Mamba CP unit tests.
  • 142 passed, 1 skipped across Magi and model-owned CP tests.
  • Ruff checks and syntax parsing passed.
  • Verified that the previous root-level CP module import paths are removed.

Local recipe collection is currently blocked before reaching this code by the environment's incompatible mlflow and system cachetools installations.

Additional Information

Related to #2861 and #2937.

@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@akoumpa
akoumpa requested a review from HuiyingLi July 22, 2026 15:08
@akoumpa akoumpa changed the title Akoumpa/refactor/cp sharder ownership refactor(cp): sharder ownership Jul 22, 2026
akoumpa added 3 commits July 22, 2026 08:39
Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
@akoumpa
akoumpa force-pushed the akoumpa/refactor/cp-sharder-ownership branch from 57214b9 to 3197656 Compare July 22, 2026 15:39
@akoumpa

akoumpa commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 3197656

self._padding_token_id = resolved._padding_token_id

@classmethod
def _from_strategy(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @akoumpa . We have a init and also a _from_strategy and then bind. I'm wondering whats the use case/if necessary?

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
Comment thread nemo_automodel/components/distributed/context_parallel/sharder.py Outdated
num_chunks=_num_chunks_value,
num_chunks=self.pp.pp_batch_size // self.pp.pp_microbatch_size if self.pp_enabled else 1,
)
train_ctx, batch = cp_sharder.shard(batch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not super sure about using init sharder+sharder.shard(). The sharder init becomes a bit confusing. Could you please explain your consideration for replacing the function call? many thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @HuiyingLi, the goal was to make ContextParallelSharder the single owner of CP strategy and mesh state, rather than keeping a procedural wrapper around it.

The object-based API has two advantages:

  1. The sharder can be cached on the trainer and reused, avoiding repeated strategy and mesh resolution. Only the batch-dependent ShardLayout is refreshed by each shard() call.
  2. The sharder owns the device mesh and related options, so shard(), shard_token_tensor(), and gather_token_tensor() do not need the mesh and other runtime arguments passed on every call.

The separate .shard(batch) operation is intentional because that is where the batch is mutated and any data-dependent layout is computed and retained for subsequent token-aligned sharding or gathering.

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
@akoumpa

akoumpa commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 47925da

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>
@akoumpa

akoumpa commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 6ccbc76

@akoumpa
akoumpa marked this pull request as ready for review July 22, 2026 22:56
@akoumpa
akoumpa requested a review from a team as a code owner July 22, 2026 22:56
@akoumpa
akoumpa merged commit 65dcfb7 into huiyingl/refactor/cp-unify Jul 22, 2026
52 checks passed
@akoumpa
akoumpa deleted the akoumpa/refactor/cp-sharder-ownership branch July 22, 2026 22:56
HuiyingLi pushed a commit that referenced this pull request Jul 23, 2026
* refactor(distributed): simplify CP sharder API

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

* refactor(distributed): group context parallel modules

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

* refactor(cp): simplify THD recipe policy

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

* refactor(cp): construct strategy sharders directly

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

* refactor(cp): remove legacy sharder construction

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

* test(cp): update VLM sharder mocks

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

---------

Signed-off-by: Alexandros Koumparoulis <akoumparouli@nvidia.com>

Signed-off-by: Alexandros Koumparoulis <153118171+akoumpa@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants