Skip to content

fix(nft): fully align NFT with DiffusionNFT - #266

Merged
Rockdu merged 3 commits into
fsdp-followups/1-docs-renamefrom
fsdp-followups/2-nft-alignment
Oct 4, 2026
Merged

Rockdu merged 3 commits into
fsdp-followups/1-docs-renamefrom
fsdp-followups/2-nft-alignment

Conversation

@Rockdu

@Rockdu Rockdu commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

Stack 2/9 of follow-ups to #263: main ← #265 ← this ← #267 ← #268 ← #269 ← #271 ← #272 ← #273 ← #274.

Aligns the NFT path with the reference implementation (NVlabs/DiffusionNFT).

What

  • NFT trains a random subset of --diffusion-nft-timestep-fraction of the sigmas per sample, instead of always dropping the smallest sigma.
  • NFT reads π_old from the EMA directly; --ref-mode now only picks the KL reference (none / lora_base / ref), and NFT supports --diffusion-kl-beta (kl_beta * mean((v_new - v_ref)^2) per pair, logged as nft_kl, renamed to kl_loss in refactor(nft): log the NFT KL term as kl_loss like Flow-GRPO #272). --loss-type nft requires --use-ema --rollout-weights ema.
  • The EMA decay schedule follows the actor's optimizer step count (EMAOptimizer.step(optimizer_step)), as DiffusionNFT's return_decay(global_step) does; the separate update_count is gone.

Why

Each item was a line-level mismatch with the reference code: a fixed dropped sigma never trains, π_old and the KL reference were one role, and the EMA ramp counted train calls instead of optimizer steps.

Validation

  • fix(nft): train a random subset of sigmas ... moves the NFT metrics; test_sd3_nft_pickscore_3xGPU and test_krea2_nft_ocr_2xGPU are re-recorded bitwise on that commit. Every other standard is unchanged.
  • refactor(nft): train against EMA pi_old ... and fix(ema): follow the actor's optimizer step count ... are metric-neutral for these recipes: π_old is the same EMA, and one optimizer step per rollout makes the step count equal the old update count. CI checks this against the standards above.

Not in this PR

  • The GRPO reward std keeps torch's default correction=1 (as miles, verl, TRL and verl-omni do; flow_grpo, DiffusionNFT and UniRL use ddof=0), since neither choice is more principled and switching would re-record every GRPO standard.

Files

  • miles/ray/data_conversion_hub/nft.py: per-sample sigma subset.
  • miles/backends/fsdp_utils/actor.py, loss_hub/{nft,flow_grpo,sft}.py: old_pred next to ref_pred; NFT KL term.
  • miles/backends/fsdp_utils/ema.py: decay from the optimizer step.
  • miles/utils/arguments.py, recipes, docs: flag semantics.
  • tests: test_loss_hub_nft.py, test_reference_models.py, test_ema_optimizer.py, test_reference_arguments.py, GPU offload worker; two re-recorded NFT e2e standards.

Checklist

  • pre-commit run --all-files passes — not run locally
  • Added/updated tests for new behaviour
  • pytest -x is green — CPU fsdp and argument tests pass locally; GPU tests run in CI
  • If launch flags changed, python3 train.py --help still parses — not run (needs the GPU image)
  • If a public flag was added, it appears in the CLI reference docs
  • If an example was added, it has a real walkthrough — n/a

@Rockdu
Rockdu force-pushed the fsdp-followups/2-nft-alignment branch from 98c4be8 to 3ab2a04 Compare September 27, 2026 13:06
@Rockdu
Rockdu added this pull request to stack #270 September 27, 2026 17:59
@Rockdu
Rockdu force-pushed the fsdp-followups/2-nft-alignment branch from 3ab2a04 to ccbeacb Compare September 27, 2026 18:10
@Rockdu Rockdu changed the title fix(nft): align NFT and GRPO with DiffusionNFT and flow_grpo fix(nft): align NFT with DiffusionNFT Sep 27, 2026
@Rockdu Rockdu changed the title fix(nft): align NFT with DiffusionNFT fix(nft): fully align NFT with DiffusionNFT Sep 27, 2026

@zhihengy zhihengy 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

@Rockdu
Rockdu merged commit cb1353c into main Oct 4, 2026
33 of 35 checks passed
OnePunchMonk pushed a commit to OnePunchMonk/miles_diffusion that referenced this pull request Oct 5, 2026
* fix(nft): train a random subset of sigmas per sample instead of always dropping the smallest

* refactor(nft): train against EMA pi_old directly and keep --ref-mode for the KL reference

* fix(ema): follow the actor's optimizer step count in the decay schedule, as DiffusionNFT does
OnePunchMonk pushed a commit to OnePunchMonk/miles_diffusion that referenced this pull request Oct 5, 2026
* fix(nft): train a random subset of sigmas per sample instead of always dropping the smallest

* refactor(nft): train against EMA pi_old directly and keep --ref-mode for the KL reference

* fix(ema): follow the actor's optimizer step count in the decay schedule, as DiffusionNFT does
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci-basic Authorize and run only basic unlabeled CI tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants