[new-model] Port Z-Image T2I to FastVideo - #1236
Conversation
There was a problem hiding this comment.
Welcome to FastVideo! Thanks for your first pull request.
How our CI works:
PRs run a two-tier CI system:
- Pre-commit — formatting (yapf), linting (ruff), type checking (mypy). Runs immediately on every PR.
- Fastcheck — core GPU tests (encoders, VAEs, transformers, kernels, unit tests). Runs automatically via Buildkite on relevant file changes (~10-15 min).
- Full Suite — integration tests, training pipelines, SSIM regression. Runs only when a reviewer adds the
readylabel.
Before your PR is reviewed:
-
pre-commit run --all-filespasses locally - You've added or updated tests for your changes
- The PR description explains what and why
If pre-commit fails, a bot comment will explain how to fix it. Fastcheck and Full Suite results appear in the Checks section below.
Useful links:
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Qwen3 model, including its configuration, architecture implementation, and registration within the model registry. It also updates the FlowMatchEulerDiscreteScheduler to include an option for reference discrete timesteps, ensuring parity with specific implementations like Z-Image. Additionally, a suite of parity tests for the encoder, scheduler, tokenizer, and VAE has been added. Feedback focuses on ensuring architectural correctness for causal masking when the attention mask is missing and improving the robustness of the weight loading logic by using safer attribute access.
| attention_mask, | ||
| dropout=self.attention_dropout if self.training else 0.0, | ||
| scaling=self.scaling, | ||
| is_causal=False, |
There was a problem hiding this comment.
The is_causal parameter is hardcoded to False. Since Qwen3 is a causal architecture (as indicated by its registration as Qwen3ForCausalLM), it should use causal masking. While sdpa_mask handles this when an attention_mask is provided, if attention_mask is None, the model will incorrectly default to bidirectional attention. Consider setting is_causal=True when attention_mask is None to maintain architectural correctness.
|
|
||
| param = params_dict[mapped_name] | ||
| weight_loader = param.weight_loader | ||
| weight_loader(param, loaded_weight, shard_id) |
There was a problem hiding this comment.
Accessing param.weight_loader directly may raise an AttributeError if the parameter belongs to a standard layer that hasn't been extended with a custom loader. It is safer to use getattr(param, "weight_loader", default_weight_loader), consistent with the implementation in the else block at line 426.
| weight_loader(param, loaded_weight, shard_id) | |
| weight_loader = getattr(param, "weight_loader", default_weight_loader) |
|
This PR has merge conflicts with the base branch. Please rebase: git fetch origin main
git rebase origin/main
# Resolve any conflicts, then:
git push --force-with-lease |
…6 encoder parity + PORT_STATUS (#1339) Co-authored-by: Mrinaal Dogra <mdogra@ucsd.edu> Co-authored-by: SolitaryThinker <wlsaidhi@gmail.com>
Summary
This PR introduces the initial Z-Image text-to-image integration work into FastVideo, focused on core component compatibility and parity validation against the local Z-Image reference implementation. It adds native Qwen3 text-encoder support, aligns scheduler timestep behavior with Z-Image reference semantics, and adds local parity tests for scheduler, tokenizer, text encoder, and VAE decode path. This is an in-progress port intended to establish correctness foundations before final end-to-end pipeline parity.
What changed
Model (Text Encoder)
model.-prefixed keys and stacked parameter remapping (QKV and gate/up projections).Scheduler
Parity tests (local)
How to test
Prerequisites
From repo root, run:
Current status / known gap