[ci] add per-method single-step training tests for fastvideo.train - #1343
Conversation
mergify
Bot
added
type: ci
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 PR merge requirementsWaiting for
This rule is failing.
|
alexzms
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces per-method GPU smoke tests for the training framework, specifically targeting the WanModel and WanCausalModel with FineTuneMethod and DiffusionForcingSFTMethod respectively. The changes include new test fixtures for minimal training configurations, dedicated test scripts that verify loss finiteness and gradient propagation, and updates to the Modal CI configuration and documentation to include these new tests in the training framework suite. I have no feedback to provide as there were no review comments.
9fdb29f to
2767c85
Compare
…R 5a-i/9)
Phase 2 / PR 5/9 of the fastvideo.train CI plan, split into a
first-half ``5a-i`` slice that establishes the per-method test
pattern. A follow-up ``5a-ii`` will layer a device-keyed
grad-norm regression on top of the same harness.
Adds two GPU smoke tests under a new
``fastvideo/tests/train/methods/`` directory:
* ``test_wan_finetune.py`` — ``WanModel`` + ``FineTuneMethod``
* ``test_wan_causal_dfsft.py`` — ``WanCausalModel`` +
``DiffusionForcingSFTMethod``
Both tests construct the method via its public constructor
(``method = FineTuneMethod(cfg=cfg.method, role_models=...)``),
build a tiny synthetic ``raw_batch`` (text embed + mask + vae
latent), and run one end-to-end step:
method.on_train_start()
loss_map, outputs, _ = method.single_train_step(batch, 0)
method.backward(loss_map, outputs, grad_accum_rounds=1)
Asserts that ``loss_map["total_loss"]`` is finite and that the
first transformer block (``model.transformer.blocks[0]``) has
trainable parameters with finite, non-zero gradients. The first
block's grad is computed *last* during backprop, so a healthy
grad there implies the full forward + chain-rule path is intact —
keeping the assertion surface to a single block keeps the
reference data tiny for the follow-up regression PR.
The harness deliberately avoids the full ``Trainer`` (no FSDP
wrap, no data loader, no checkpointing) so the tests stay focused
on the method-level wiring that the model-loading tests in
``tests/train/models`` don't cover.
CI plumbing:
* ``fastvideo/tests/modal/pr_test.py``: add
``./fastvideo/tests/train/methods`` to the
``run_train_framework_tests`` pytest path, and add a matching
``--ignore`` entry in ``run_unit_test`` so the CPU runner doesn't
pick it up.
* ``docs/contributing/testing.md``: mention the new ``methods/``
subdirectory in the Train Framework Tests entry.
Two new fixture YAMLs under ``fastvideo/tests/train/fixtures/``
mirror the structure of the existing 4/9 fixtures but flip
``trainable`` to ``true`` and include the minimum
``training.distributed`` + ``training.optimizer`` + ``training.loop``
keys needed by the method's ``__init__`` and ``on_train_start``.
2767c85 to
8dfc13a
Compare
|
Hi @alexzms — this is a code review from one of @SolitaryThinker's AI reviewer agents (Gob). I run these to help triage PRs but @SolitaryThinker hasn't personally verified every finding — please ping @SolitaryThinker if anything below is off. TL;DRPer-method test template is clean (public constructors, synthetic raw_batch, layer-0 grad assertion) and the Modal/ Verdict: ship-with-fixes
Findings (must-fix + persistent should-fix only)[S1] Both new tests will fail at construction because
|
Both fixtures intentionally omit training.data.data_path, so FineTuneMethod/DiffusionForcingSFTMethod construction hit a FileNotFoundError in init_preprocessors before the train step ran. The tests feed a synthetic raw_batch, so the dataloader is unused — stub build_parquet_t2v_train_dataloader in each test. Also align the synthetic text_attention_mask dtype (float32) with the production dataloader.
|
/test train-framework |
|
Hi @alexzms — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off. TL;DRDifferential re-review at Verdict: approve
Prior findings status at c3bbb38
Worth noting: both fixes include explanatory comments (the monkeypatch block explains why the dataloader is unused on the synthetic- The monkeypatch target string ( — Gob (@SolitaryThinker's AI reviewer). Full review archived locally. |
2 participants
Summary
Phase 2 / PR 5/9 of the
fastvideo.trainCI plan, split into a 5a-i slice that establishes the per-method test pattern. A follow-up 5a-ii will layer a device-keyed grad-norm regression on top of the same harness.Adds two GPU smoke tests under a new
fastvideo/tests/train/methods/directory:test_wan_finetune.py—WanModel+FineTuneMethodtest_wan_causal_dfsft.py—WanCausalModel+DiffusionForcingSFTMethodBoth tests build the method via its public constructor, feed a tiny synthetic
raw_batch(text embed + mask + vae latent), and run one end-to-end step:The harness deliberately avoids the full
Trainer(no FSDP wrap, no data loader, no checkpointing) so the tests stay focused on the method-level wiring that the model-loading tests intests/train/modelsdon't already cover.Asserts
loss_map["total_loss"]is finite.model.transformer.blocks[0]has trainable params with finite, non-zero gradients.The first transformer block's gradient is computed last during backprop, so a healthy grad there implies the full forward + chain-rule path is intact. Keeping the assertion surface to a single block keeps the reference data tiny for the follow-up regression PR.
CI plumbing
fastvideo/tests/modal/pr_test.py:run_train_framework_testspytest path now includes./fastvideo/tests/train/methods.run_unit_testadds a matching--ignorefor the new dir.docs/contributing/testing.md: mention the newmethods/subdirectory in the Train Framework Tests entry.Fixtures
Two new YAMLs under
fastvideo/tests/train/fixtures/, mirroring the existing 4/9 fixtures but withtrainable: trueand the minimumtraining.distributed/training.optimizer/training.loopkeys needed by the method's__init__andon_train_start:wan_t2v_finetune_min.yamlwan_causal_t2v_dfsft_min.yaml(withchunk_size: 3,num_latent_t: 6)Test plan
python -m pytest --collect-only fastvideo/tests/train/methods/collects both tests cleanly.pre-commit runon changed files (codespell + PyMarkdown checked; yapf/ruff/mypy are intentionally excluded underfastvideo/tests/)./test train-framework— first full GPU CI run.Plan context
Followup PRs will incrementally fill the (model, method) matrix:
dmd2,self_forcing,kd(needs tiny teacher/fake-score fixtures)