Skip to content

refactor(fsdp): DiffusionLossContext stops carrying loop state - #368

Merged
Rockdu merged 24 commits into
mainfrom
dmd2/refactor-c-ctx-loop-state
Nov 11, 2026
Merged

Rockdu merged 24 commits into
mainfrom
dmd2/refactor-c-ctx-loop-state

Conversation

@Rockdu

@Rockdu Rockdu commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #367 (the hook-registry PR), which sits on #343: the diff here is the 4 commits above it.

What

  • DiffusionLossContext stops carrying loop state. microbatch_id and optim_step_idx become required keyword arguments of every prepare hook (and optim_step_idx of every formula), passed by the two trunk loops; the list the DMD2 critic formula fills for the train-output hook is owned by _train_core and passed to the formulas and to train_output(ctx, train_pairs, train_output_inputs).
  • The context is frozen (@dataclass(frozen=True, eq=False)): a rollout-level, read-only view; nothing writes it after construction.
  • --loss-type dmd2 rejects --diffusion-recompute-old-log-prob (no log-prob ratio to recompute; with a real step index the recompute loop would run the critic's scoring window twice), like sft_loss and nft already do.

Why

The planned micro-batch prefetch (prepare of micro-batch n+1 while the loss of n runs) races on any per-micro-batch field shared through the context, and a hook-to-hook channel through the context is invisible in the call graph. With the counters and the list as arguments, every per-call fact is in the signature and the context can be shared freely. Numbering is unchanged: training counts from 0; the old-log-prob recompute of windows 1.. reuses their training forwards' numbers.

Not in this PR: moving the DMD2 teacher forward into the trunk and deleting the train-output hook (left to the DMD2 stack's author).

Validation

CPU, on this head:

check result
tests/fast (minus the Mac-only distributed / ray-socket cases) 634 passed, 3 skipped
one-shot comparison against the hook-registry PR's tree (ctx-field version): sft / nft / dmd2-critic prepares over 4 micro-batch ids x 2 step indices, the critic formula over 2 steps x 2 micro-batches, score_last_step_samples 28 checks bitwise equal
test_train_input_hook.py the training loop and the recompute loop pass the expected (microbatch_id, optim_step_idx, write_old_log_prob) per forward; the train-output hook receives the same list object every forward appended to
test_reference_models.py the real _forward_train_pair_batch hands prepare {microbatch_id, optim_step_idx, pad_to_len} and the formula optim_step_idx and the same list
pre-commit run --all-files passes

GPU (4x H200, sglang-miles head), before the GPU runs were paused, on the same miles/ code except two argparse help strings: test_sd3_ocr_grpo_2xGPU 8/8, test_sd3_nft_pickscore_3xGPU 10/10, test_h3_t2va_sft_2xGPU 2/2 metric series bitwise.

Behaviour ledger (intentional changes): a custom prepare with the old signature prepare(ctx, batch, *, pad_to_len=None) fails at the first micro-batch with TypeError: unexpected keyword 'microbatch_id'; a custom train-output hook with the old two-argument signature fails the same way; a custom formula written as documented (with **kwargs) keeps working; reading ctx.microbatch_id / ctx.optim_step_idx / ctx.train_output_inputs is an AttributeError; writing a context field is a FrozenInstanceError. Everything else is bitwise.

Files

  • miles/backends/fsdp_utils/loss_hub/types.py: three fields removed; context frozen
  • miles/backends/fsdp_utils/actor.py: the two loops pass the counters; one list per train call
  • miles/backends/fsdp_utils/loss_hub/{sft,nft,flow_grpo,dmd2}.py: hook signatures
  • miles/backends/fsdp_utils/loss_hub/__init__.py: LossHooks contract
  • miles/utils/arguments.py: dmd2 rejects the recompute flag; help strings
  • docs/user-guide/customization.md: hook signatures and the numbering rule
  • tests/fast/...: call sites updated; new wiring, accumulator-identity and frozen-context tests; tests/fast-gpu/backends/fsdp_utils/_reference_models_worker.py call sites updated

Checklist

  • pre-commit run --all-files passes
  • Added/updated tests for new behaviour
  • pytest -x is green (tests/fast on CPU; tests/fast-gpu was updated but not run on a GPU)
  • If launch flags changed, python3 train.py --help still parses (train_diffusion.py --help)
  • If a public flag was added, it appears in the CLI reference docs (no flag added)
  • If an example was added, it has a real walkthrough (no example added)

@Rockdu Rockdu added run-ci-fsdp Run FSDP domain tests on this PR run-ci-e2e Run e2e metric-regression tests on this PR labels Nov 10, 2026
zhihengy and others added 15 commits November 11, 2026 06:53
`train` returned its train output directly, so the driver's ray.get on the
train refs pulled every rank's output into the controller before the next
group read it again from the object store. Return `Box(ray.put(output))`
like the rollout shards: the driver gets a small box, the payload stays in
the store until the paired rank of a later group unboxes it.

Also note the train-output hook as a `--teacher-load` consumer in
docs/fsdp_model_roles.md, which cli-reference.md already did.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
train returns Box(ray.put(output)); the driver discarded its ray.get and
passed the task refs on, so the consumer's box.inner hit an ObjectRef.
Keep the boxes the way the rollout shards are kept.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
…tprocess

Review on #338: `custom_train_input_postprocess_func` says what the hook
does, a post-processing step on the converted train pairs. The flag
follows the prepare hook's pattern: `--custom-train-input-postprocess-path`
sets `args.custom_train_input_postprocess_path`, loaded into
`self.custom_train_input_postprocess_func`. Docs and the hook test follow.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
#338 renamed the hook to --custom-train-input-postprocess-path; the DMD2
defaults and the critic override still set custom_train_input_function_path,
so --loss-type dmd2 failed in parse_args.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
miles/ray/rollout.py imports imageio_ffmpeg to mux the eval audio; the
GPU image had it only as a transitive dependency and the hosted CPU
suite not at all, so tests/fast/rollout/test_eval_media.py failed to
import there.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
Whether a hook forwards the teacher is the hook's business; the argument
layer cannot tell from --loss-type and the hook paths who will read it,
so the check only ever guessed. --teacher-load is now accepted under every
loss type (dmd2 still requires it).
Stop filling the --custom-*-path arguments on behalf of nft / dmd2 and
stop overriding them per trainer role: a group's prepare, formula,
train-input and train-output hooks come from loss_hub.LOSS_HOOKS keyed by
(loss_type, role), the way miles core's get_loss_function does. Only
custom_loss reads the path flags. The critic overrides shrink to the six
values that really differ (checkpoint, lr, load/save dirs, steps, batch).
A path given under any other loss type would be silently ignored now that
the table selects the hooks, so fail at parse time instead.
DMD2 has no log-prob ratio to recompute, and once the recompute loop
passes its real step index the critic's last window would run its
scoring prepare and formula a second time. sft_loss and nft already
reject the flag.
microbatch_id and optim_step_idx were written onto DiffusionLossContext
before every forward and read back by the hooks; the planned micro-batch
prefetch would race on them. They are now required keyword arguments of
every prepare and formula hook, passed by the two trunk loops; the
numbering is unchanged (training from 0, the old-log-prob recompute of
windows 1.. reuses their training forwards' numbers).
The DMD2 critic formula appended its scoring inputs to a list on the
context and the train-output hook read them back: a hook-to-hook channel
invisible in the call graph. The trunk now owns one list per train call
and passes it to the formulas and to the train-output hook.
Nothing writes the context after construction any more: it is the
rollout-level, read-only view the hooks share. eq=False keeps identity
comparison; a synthesized hash would fail on the dict fields.
@Rockdu
Rockdu force-pushed the dmd2/refactor-a-hook-registry branch from fa6087f to 972e01e Compare November 11, 2026 13:03
@Rockdu
Rockdu force-pushed the dmd2/refactor-c-ctx-loop-state branch from b6aee00 to 60acc02 Compare November 11, 2026 13:03
@Rockdu Rockdu removed run-ci-e2e Run e2e metric-regression tests on this PR run-ci-fsdp Run FSDP domain tests on this PR labels Nov 11, 2026
@Rockdu
Rockdu removed this pull request from stack #374 November 11, 2026 15:54
@Rockdu
Rockdu changed the base branch from dmd2/refactor-a-hook-registry to main November 11, 2026 15:55
@Rockdu
Rockdu merged commit 74c66d6 into main Nov 11, 2026
8 of 14 checks passed
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