fix(nft): give NFT its own timestep hook instead of sharing process_timestep_as_input - #155
Merged
Merged
Conversation
Rockdu
force-pushed
the
fix/nft-sigma-input-hook
branch
from
August 11, 2026 21:43
386a16f to
e123e14
Compare
This was referenced Aug 11, 2026
Rockdu
marked this pull request as ready for review
August 12, 2026 00:59
…imestep_as_input PR radixark#125 routed NFT's prepare through process_timestep_as_input, but NFT's input is not a trajectory timestep. Its pairs carry a sigma in [0, 1] read straight off scheduler.sigmas, so the rescaling runs the opposite direction from flow-GRPO and SFT: the families that pass the timestep through unchanged there have to scale up to the scheduler range here, and qwen_image, which divides there, must pass through here. The old needs_timestep_scaling branch in nft.py encoded exactly that inversion; folding it into the shared hook required pre-multiplying by num_train_timesteps, which qwen_image then divides back out. That round trip is not the identity in float32. Multiplying by 1000 and dividing by 1000 lands off by a ULP on ~2% of sigmas, against a hook whose whole purpose is reproducing the rollout DiT's arithmetic bit-for-bit. Add process_sigma_as_input(sigmas, *, num_train_timesteps): the base scales up to the scheduler range (sd3, wan2_2, ltx), qwen_image passes the sigma through. Every family recovers its pre-radixark#125 tensor exactly. Also drop the now-dead needs_timestep_scaling from ltx.py and reattach the wan2_2 comment that radixark#125 orphaned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… not the scheduler range sglang-d's DiT divides by the literal 1000 (runtime/models/dits/qwen_image.py) while its flow-match scheduler converts sigma with num_train_timesteps. The two constants are different facts that happen to share a value; note which is which so neither hook gets 'fixed' into reading the other. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the full rationale into the process_sigma_as_input docstring and leave one-line constraints at the call sites; drop the narrating lines and the wan2_2 comment that outlived the attribute it annotated and had drifted onto component_for_timestep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… comments back The name now carries what the comments were spelling out: sigma in, timesteps input out. Leaves one line per non-local constraint -- why the hook is separate, and why qwen_image's 1000 must not become num_train_timesteps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rockdu
force-pushed
the
fix/nft-sigma-input-hook
branch
from
August 12, 2026 07:44
e123e14 to
bc29eb1
Compare
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.
What
process_sigma_as_timesteps_input(sigmas, *, num_train_timesteps)toTrainPipelineConfig: the base scales up to the scheduler range,qwen_imagepasses the sigma through.prepare_nft_batchcalls it instead of pre-multiplying intoprocess_timestep_as_input.needs_timestep_scalingleft onltx.py, and thewan2_2.pycomment that outlived the attribute it annotated.Why
#125 folded three prepare hooks onto one
process_timestep_as_input. NFT does not belong on it: its input is not a trajectory timestep.NFT's pairs carry a sigma read straight off
scheduler.sigmas(data_conversion_hub/nft.py:72), so σ ∈ [0, 1], while flow-GRPO and SFT pass a timestep in the scheduler range. The rescaling therefore runs the opposite direction, which is exactly what the oldneeds_timestep_scalingbranch innft.pyencoded — inverted relative to its two siblings:flow_grpo/sftnftTrue(qwen_image)t / NtunchangedFalse(sd3, wan2_2, ltx)tunchangedt * NSharing one hook required NFT to pre-multiply by
num_train_timestepsso the shared hook had a scheduler-range value to work on. Forqwen_imagethat hook then divides it straight back out, and(σ * 1000) / 1000is not the identity in float32 — it lands a ULP off on ~2% of sigmas (measured over 200k samples). That is the exact rewrite #125 set out to prevent; its own test says "any rewrite of the expression drifts ULPs".Behaviour
σ is the NFT pair sigma, N the scheduler range.
σ * Nσ * Nσ * Nσ(σ * N) / 1000σOnly
qwen_image+ NFT is affected; every family recovers its pre-#125 tensor bit-for-bit. The identity families were never wrong, which the tests assert explicitly — revertingnft.pyalone leavestest_sd3_style_family_gets_the_scheduler_rangegreen and fails only the qwen and wiring cases.Why two hooks rather than one signature
The two constants are different facts that happen to share a value, and #125's hardcoded
1000is right:num_train_timesteps— a scheduler property (scheduling_flow_match_euler_discrete.py:148,239).1000— a model property; sglang-d's DiT divides by the same literal (runtime/models/dits/qwen_image.py:1536) and diffusers leaves that division to its caller.Reading
num_train_timestepsinprocess_timestep_as_input(the pre-#125 behaviour) would diverge from the rollout under any other scheduler range. Noted in a comment so neither hook gets "fixed" into reading the other.Files
configs/train_pipeline_config.py— hook added, base scales bynum_train_timestepsconfigs/qwen_image.py— passes the sigma throughloss_hub/nft.py— calls the sigma hook, no pre-multiplyconfigs/ltx.py,configs/wan2_2.py— refactor(fsdp): upgrade needs_timestep_scaling to process_timestep_as_input in TrainPipelineConfig #125 leftoverstests/.../test_train_pipeline_config_registry.py—TestProcessSigmaAsTimestepsInputtests/.../test_loss_hub_nft.py—TestPrepareNftBatchThe bit-exactness guards use
torch.equal, notallclose: the buggy composition passesallclose.0.8474337458610535is a sigma the 1000-round-trip does not return unchanged.Stack
Checklist
pre-commit runpasses — on the touched files (black/ruff/isort/autoflake)pytestis green —tests/fastmatches cleanmain; the 4 failures there (test_hybrid_shard_mesh,test_lora_args,test_metric_buffer_dist) reproduce onmainand are macOS c10d socket issuessglangstubbed out; CI is the real check.python3 train.py --helpstill parses — no flag changes