Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No actionable PR-introduced issue is established. The reported SP2 scaling discrepancy remains a separate diagnostic, not a demonstrated CFG regression. Security Architecture Review
Pre-merge checks |
|
ecbdb05 to
b877e04
Compare
Coach257
left a comment
There was a problem hiding this comment.
Thanks for the PR. I checked out b877e04 and reviewed it locally. Overall the objective is implemented correctly and the default path is unchanged; the inline comments are mostly about a sync-contract regression, the sigma schedule's effective behavior, and data/doc gaps.
Verified locally (CPU)
cfg_calibrated_mse: recomputed independently thatk=0equalsMSE(p, s*y - (s-1)*stopgrad(u))and thatureceives no gradient. Signs are consistent (p,uare both-logits, target isnoise - clean).prepare_unconditionalreuses the already-noised video/audio rows and reference anchors and remaps per-token timesteps; no extra RNG draws.replace_text_layout/ the geometry check matchbuild_packed_fl2va/build_packed_ref2va(text on axis 0 as0..text_len, visual/audio coordinates offset bytext_len).training_cfg_scale=1keeps the old loss and RNG sequence;pack_samplesignores the new keys; disabling checkpointing for theno_gradbranch is correct since the DiT only honors the explicit flag.tests/models/test_minimax_h3_packing.py: 82 passed. All H3 tests +tests/trainer/test_dit_microbatch.py: 114 passed, 21 skipped (GPU-only).make qualitypasses.
Not verified: anything on GPU (including test_no_implicit_sync_in_minimax_h3_forward_backward), FSDP2/SP, or end-to-end training.
b877e04 to
b2fd747
Compare
Coach257
left a comment
There was a problem hiding this comment.
Thanks for the quick update. I re-reviewed b2fd747; the previous round is addressed:
- Implicit syncs: fixed. On an A100,
test_no_implicit_sync_in_minimax_h3_forward_backwardpasses all 12 cases (off/per_sample/shared_emptyx single / packed x SDPA / FA2). As a negative control I injected onebool(tensor.all())intoprepare_unconditional; both CFG cases fail as expected, so the gate really covers the new path. sigmavsconstant(now the recipe default), the documented scale distribution, the" "negative prompt, the cache helpers, and removingvideo_sigma_bucket_weights/training_cfg_loss_modeall look good. CPU tests (H3 + DiT trainer + resume): 122 passed;make qualitypasses.
Two follow-up suggestions below. Together they make the PR smaller and remove the shared DiTTrainer change entirely:
- Remove
shared_empty. It is not a cheaper version ofper_sample; it is a different CFG objective (details inline). - Drop per-step value validation. Validate once when the cache is written, and keep only metadata checks at train time. Metadata checks run fine on device tensors without syncs, so
prepare_condition_batch,_cfg_batch_tokenand theDiTTrainer.preforwardhook are no longer needed. No legacy-layout compatibility is required.
With the trainer change gone, the [model] title would be accurate again (right now the commit says [model, trainer], the PR title says [model]). A short multi-GPU FSDP2 smoke run is still the remaining open item from the last round.
|
@Coach257 Thank you for the prompt and detailed review, and for taking the time to run the checks yourself! I’m addressing your latest comments locally and will update the PR after validation.
|
|
@blacklist-twx Thanks for the detailed reply. The proposed scope sounds good to me:
On effectiveness: feel free to keep running your experiments on the visual-context-free negative. Sharing the results here (or in the docs) once you have them would be great, but it doesn't need to block this PR. On the multi-GPU smoke test: a reduced-size H3 model with synthetic cached inputs is sufficient for correctness. The useful checks are loss / grad-norm parity on identical inputs for single GPU vs. FSDP2, and SP off vs. on, with CFG both off and on, plus confirming that the LoRA parameters actually update. Pretrained weights and real samples aren't needed for this PR. |
Keep visual context by default and document opt-in text-only negatives. Move value validation to cache creation and remove the shared embedding loader and common Trainer hook. Record four-GPU smoke results: forward loss parity and LoRA updates pass for SP1/SP2; the unmodified SP2 path retains a raw gradient scaling discrepancy.
|
@Coach257 Thanks for the review. I’ve pushed the updates addressing your comments and added the four-GPU FSDP2/SP test report, including its limitations. The 93 focused tests and documentation build pass. Please take another look when convenient. Any further suggestions are welcome. |
…ision Merge main 62beaef, which landed the CFG-calibrated Ref2VA training (ByteDance-Seed#1258), the Ulysses gradient scaling (ByteDance-Seed#1281) and the Flux SP fix (ByteDance-Seed#1248). Only the DiT wrapper import conflicted: keep the official FP32 projection import alongside main's CFG objective import. Every other file auto-merged, and both sides' changes are preserved: - the six FP32 projections, the FP32 time MLP, the AdaLN cast order and the rejection of FP32-rounded timesteps/positions - the CFG negative branch, the SP gradient compensation and the async all-to-all loop reset Validation: tests/models/test_minimax_h3_packing.py and tests/parallel/ulysses/test_all_gather.py pass 118 and skip 7 on CPU/Gloo; make quality passes. A BF16 CFG probe on the merged tree matches serial and packed loss and gradients to 6.2e-4 relative error with the FP32 islands preserved. CFG under FSDP2/BF16 and the accelerator-only kernel paths are still unverified here.
What does this PR do?
MiniMax H3 is CFG-distilled. Ordinary flow-matching SFT fits its conditional prediction directly to a raw velocity target and can erode its distilled guidance behavior during prolonged fine-tuning. This PR adds opt-in CFG-calibrated flow matching to the existing offline FL2VA and prepared visual Ref2VA training paths, plus a Ref2VA LoRA recipe.
Given conditional velocity
p, detached unconditional velocityufrom the same current model, and the raw FM targety = noise - clean:Both predictions use the same noisy latents, timesteps, and DiT reference latent anchors. Only the conditional prediction receives gradients. Curvature power
kcontrols the1/s**kcurvature factor:2retains the inverse-CFG attenuation and0removes it. These are settings of one objective, not separate methods. Scale1retains the existing single-forward FM path.The approach is informed by DiffSynth-Studio's H3 training and AI Toolkit's guidance-loss implementation. Calibration mitigates objective mismatch; it does not guarantee generation quality or establish the original model's distillation scale or conditioning-drop policy.
Enabling calibration adds one no-gradient DiT forward. Our prior training measurements showed about 30% additional step time; this is not a throughput benchmark of this public branch, and overhead depends on hardware and branch sequence lengths.
Checklist Before Starting
[model] feat: ...format.Test
tests/models/test_minimax_h3_packing.pyandtests/models/test_model_forward_no_implicit_sync.py; both are listed in the GPU/NPU unit workflows.1.35899e-5. SP1 gradients matched the reference. SP2/reference gradient-norm ratios were approximately0.5with CFG both off and on: no additional CFG-specific norm-scaling discrepancy was observed. This does not establish full SP gradient equivalence; the scaling observation remains a separate unresolved diagnostic item, without attribution to a particular upstream change.make quality,git diff --check, and documentation task-path checks passed.DiTTrainerjob. It does not validate pretrained generation quality, real encoder execution, BF16, multi-node behavior, checkpoint resume, or long-run convergence. The full repository test suite was not validated; shared Trainer/resume tests on this CPU environment encounter a missing CUDA Triton dependency.API and Usage Example
Paired negative caches are prepared through
add_unconditional_cache():" "(one space), retaining the sample's Qwen visual context.drop_visual=Truedrops Qwen visual context while retaining DiT reference latent anchors. This changes the guidance direction; it is not an equivalent or merely cheaper form of the default.unconditional_prompt_embedsandunconditional_packed, and the same training loss. No shared-embedding loader,cfg_unconditional_mode, or unconditional file-path setting is required.The optional
sigmaschedule usess = 1 + (training_cfg_scale - 1) * sigmaseparately for video/audio, wheresigma = 1 - tis the effective noise level from the existing schedulers. At scale 4,sigma=0.1givess=1.3, whilesigma=0.9givess=3.7: stronger calibration at high noise, with more room for ordinary FM learning at low noise. Existing shifts affect how often each region is sampled. This is a protection/learning tradeoff, not a quality guarantee or a new noise sampler. The recipe defaults toconstant.Design & Code Changes
DiTTraineris unchanged from the PR base.Checklist Before Submitting
tasks/scripts moved or renamed; documentation task-path check passed.Summary by CodeRabbit
New Features
Documentation