[Feat] Add Ovis-Image-7B text-to-image pipeline - #1117
Conversation
Summary of ChangesHello @HenryDzy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the FastVideo framework by integrating the Ovis-Image-7B text-to-image model. It introduces new model architectures for the diffusion transformer and text encoder, along with their respective configurations and pipeline implementations. The changes enable users to perform high-quality text-to-image generation and fine-tune the Ovis-Image model within the FastVideo ecosystem. Additionally, the PR includes important refactorings to the model and pipeline registration systems, improving modularity and maintainability, and adds comprehensive test coverage to ensure the stability and correctness of the new features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a great pull request that adds comprehensive support for the Ovis-Image-7B text-to-image model. The changes include native model implementations, configurations, a new pipeline, and a full suite of tests, which is excellent. The refactoring in the model and pipeline registries also helps to simplify the codebase. I've identified a few critical issues related to type correctness in configurations and hardcoded paths in tests and examples that need to be addressed. Once these are resolved, this will be a very solid contribution.
| def import_pipeline_classes( | ||
| pipeline_types: list[PipelineType] | PipelineType | None = None | ||
| ) -> dict[str, dict[str, type[ComposedPipelineBase] | None]]: | ||
| pipeline_types_key: tuple[PipelineType, ...] | PipelineType | None | ||
| if isinstance(pipeline_types, list): | ||
| pipeline_types_key = tuple(pipeline_types) | ||
| else: | ||
| pipeline_types_key = pipeline_types | ||
| return _import_pipeline_classes_cached(pipeline_types_key) | ||
|
|
||
|
|
||
| @lru_cache | ||
| def _import_pipeline_classes_cached( | ||
| pipeline_types: tuple[PipelineType, ...] | PipelineType | None = None | ||
| ) -> dict[str, dict[str, type[ComposedPipelineBase] | None]]: | ||
| ) -> dict[str, dict[str, dict[str, type[ComposedPipelineBase] | None]]]: |
There was a problem hiding this comment.
The @lru_cache decorator requires all arguments to be hashable. The pipeline_types argument is typed as a list, which is not hashable and will raise a TypeError at runtime if a list is passed. To fix this, the function signature should be changed to accept a tuple instead of a list.
| def import_pipeline_classes( | |
| pipeline_types: list[PipelineType] | PipelineType | None = None | |
| ) -> dict[str, dict[str, type[ComposedPipelineBase] | None]]: | |
| pipeline_types_key: tuple[PipelineType, ...] | PipelineType | None | |
| if isinstance(pipeline_types, list): | |
| pipeline_types_key = tuple(pipeline_types) | |
| else: | |
| pipeline_types_key = pipeline_types | |
| return _import_pipeline_classes_cached(pipeline_types_key) | |
| @lru_cache | |
| def _import_pipeline_classes_cached( | |
| pipeline_types: tuple[PipelineType, ...] | PipelineType | None = None | |
| ) -> dict[str, dict[str, type[ComposedPipelineBase] | None]]: | |
| ) -> dict[str, dict[str, dict[str, type[ComposedPipelineBase] | None]]]: | |
| def import_pipeline_classes( | |
| pipeline_types: tuple[PipelineType, ...] | PipelineType | None = None | |
| ) -> dict[str, dict[str, dict[str, type[ComposedPipelineBase] | None]]]: |
| if isinstance(pipeline_types, list): | ||
| pipeline_types_to_scan = [ | ||
| pipeline_type.value for pipeline_type in pipeline_types | ||
| ] |
There was a problem hiding this comment.
Following the change to the function signature to accept a tuple for caching purposes, this check should be updated to look for a tuple instead of a list.
| if isinstance(pipeline_types, list): | |
| pipeline_types_to_scan = [ | |
| pipeline_type.value for pipeline_type in pipeline_types | |
| ] | |
| if isinstance(pipeline_types, tuple): | |
| pipeline_types_to_scan = [ | |
| pipeline_type.value for pipeline_type in pipeline_types | |
| ] |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
This pull request has been automatically marked as stale because it has not had any activity within 60 days. It will be automatically closed if no further activity occurs within 14 days. Leave a comment if you feel this pull request should remain open. Thank you! |
mergify
Bot
added
type: feat
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.
|
github-actions
Bot
added
unstale
|
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 |
# Conflicts: # fastvideo/configs/models/dits/__init__.py # fastvideo/configs/models/encoders/__init__.py # fastvideo/models/registry.py # fastvideo/pipelines/pipeline_registry.py # fastvideo/pipelines/stages/causal_denoising.py # fastvideo/pipelines/stages/denoising.py # fastvideo/pipelines/stages/matrixgame2_denoising.py
# Conflicts: # fastvideo/configs/models/dits/__init__.py # fastvideo/configs/models/encoders/__init__.py # fastvideo/configs/models/encoders/qwen3.py # fastvideo/models/encoders/qwen3.py # fastvideo/models/registry.py # fastvideo/pipelines/stages/text_encoding.py
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.
|
This comment was marked as resolved.
This comment was marked as resolved.
SolitaryThinker
left a comment
SolitaryThinker
left a comment
There was a problem hiding this comment.
Hi @HenryDzy — automated review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off.
Verdict: COMMENT (model scope, open PR). The add-a-model flow is complete and the weight-load contract — usually the failure point for a new model — is verified sound. No blocker; two majors to resolve before merge, plus minor cleanups.
First, a positive worth stating because it looks like the classic bug: the empty param_names_mapping is correct here, not a silent weight-skip. I derived the DiT's named_parameters() from __init__ and diffed it against the HF AIDC-AI/Ovis-Image-7B transformer safetensors index — 583/583 keys match exactly, 0 missing / 0 extra — and the DiT loader is strict (fsdp_load.py:362-364 raises on an unmapped checkpoint key; :405-413 raises on an unloaded model param), so a gap would hard-fail rather than skip. The module names mirror Diffusers 1:1 by design. Same story for the Qwen3 encoder (un-prefixed ckpt keys match the model params; strictness enforced at component_loader.py:461-466).
Major
-
SP-unsafe: RoPE computed at full sequence length but applied to an SP-sharded tensor → breaks at
sp_world_size > 1.fastvideo/models/dits/ovisimage.py:543-556buildsimg_cos/img_sin(andjoint_cos/joint_sin) for the full sequence, then shardsimgviasequence_model_parallel_shard(img, dim=1)._apply_rope(:102-111) then broadcasts full-length cos/sin[1, full_seq, 1, head_dim]againstq/kof shape[B, seq/sp, heads, head_dim]— a shape mismatch on every rank when sp>1, in both the double-block (:246) and single-block (:301) paths. The single-block path also doescat([txt, img], dim=1)(:384) mixing a full-lengthtxtwith a shardedimg, which is logically inconsistent across ranks.shard()no-ops at sp=1 (parallel_state.py:325), so the SSIM test (sp_size=1) and smoke test (num_gpus=1) never hit it — butsp_sizedefaults tonum_gpus(fastvideo_args.py:765), so any 2+ GPU run without an explicitsp_size=1breaks. Reference DiTs shard RoPE per-rank (wanvideo.py:673,715-719) or skip the manual shard and letDistributedAttentionhandle the all-to-all (flux_2.py). Fix: shard cos/sin (andtxt) to match the local segment, or drop the manual shard/all-gather and rely onDistributedAttentionlike Flux2 — then add ansp_size=2parity/SSIM run. If multi-GPU SP is out of scope for v1, gate it withassert get_sp_world_size() == 1in__init__so it fails loud. -
CI is red on the encoder/unit jobs.
fastcheck-passedfails viamicroscope-encoder-testsandmicroscope-unit-tests(exit 1);microscope-transformer-testsandpre-commitare green. A PR adding a text encoder with the encoder-test job failing is a real signal, and the PR body has no "expected to fail / seeded in follow-up" note. Please post the failing job output (or run the encoder + unit/testscopes) and confirm it's unrelated to the newQwen3Modelbefore merge.
Minor
onboarding.md(repo root, +137) looks like personal dev-scratch (a conda bootstrap, aPYTHONPATHhack, branch notes) — please remove it; the local-test setup already lives intests/local_tests/ovis_image/README.md.fastvideo/configs/ovis_image_7b_t2i_pipeline.jsonappears unreferenced — theOvisImageT2IConfigdataclass is the real config path. Drop it if it's not a documented override, to avoid two sources of truth.fastvideo/models/dits/ovisimage.py:433hardcodes_compile_conditions = [], while the arch config defines the block conditions and every peer DiT wires them through (flux_2.py:853,wanvideo.py:563). As written,torch.compileshards nothing for Ovis — likely unintended.fastvideo/tests/ssim/test_ovis_image_similarity.pyhas a single prompt (one hash from a false green); consider adding one more and seeding its reference. Thefull_quality(1024²) tier also has no Ovis reference seeded yet (only thedefault/L40S tier).- The PR body's file lists are stale (they cite
tests/transformers/test_ovisimage.py,tests/encoders/test_qwen3_encoder.py, and stage edits that aren't in the diff). Tests now live undertests/local_tests/ovis_image/; the stages touched aredecoding.py/text_encoding.py/timestep_preparation.py.
The local parity-test suite is genuinely strong — transformer/encoder/VAE/end-to-end tests all run through the production loaders vs official references with tight tolerances. The main gap is that they're all under tests/local_tests/ and skipped in CI, so the only CI-visible model test is SSIM (currently blocked by the red CI above). A CI-visible SSIM/parity run on a seeded tier would close that out.
Training (ovis_image_training_pipeline.py, the _compile_conditions/FSDP interaction) is out of model scope — deferring to the training reviewer.
— Gob (@SolitaryThinker's AI reviewer).
SolitaryThinker
left a comment
SolitaryThinker
left a comment
There was a problem hiding this comment.
Full review at head b2751be (components + pipeline lanes, findings adversarially verified). The port itself is in good shape: native DiT, TP-layer encoder, real parity evidence through the production loaders (bit-exact timestep schedule, latent-drift gate, documented non-skip passes), SSIM references already seeded on HF, and the HF repo is plain diffusers layout so no conversion script is needed.
The blocker is the June-10 merge of main, not the port. The merge resolved badly and this branch now REVERTS main functionality:
fastvideo/models/registry.pylost the model auto-discovery mechanism plus the GameCraft, CLIPTextModelWithProjection, and upsampler registrations — that's what reds your encoder lane (T5 resolves to the hf-wrapper class), and merged it would break GameCraft, SD3.5, StableAudio, Magi, and LTX2 upsampling across inference and both training stacks.training/__init__.pydropped theLTX2TrainingPipelineexport.- The schema-parity inventory is stale (the 3 unit-test failures clear on rebase — main fixed those in #1446, not your bug).
Fix: rebase onto main (branch is CONFLICTING, 93 commits behind), take main's side of registry.py/training/__init__.py/the inventory yaml, and limit registry.py to your two additive Ovis lines.
After that, two port items:
- SP > 1 crashes despite the docstring claim: RoPE cos/sin stay full-length while
imgis sharded, single-stream blocks concat replicatedtxtwithout thereplicated_*kwargs, and pre-all-gather padding is never trimmed. Fix or drop the SP claim. - The new
Qwen3Modelduplicates the Flux2 Qwen3 stack that landed on main after this PR was written (~350 lines). Consolidate onto the existing decoder layers — a config-level delta covers the Ovis chat-template path.
Smaller: add Qwen3Model to EntryClass; remove the dead duplicate AutoencoderKL registry key, root-level onboarding.md, and the unreferenced ovis_image_7b_t2i_pipeline.json; refresh the PR description (the denoising.py changes it describes no longer exist at head).
|
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 |
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
3 participants
Adds native FastVideo support for Ovis-Image-7B
New files
Models & configs
fastvideo/models/dits/ovisimage.py— NativeOvisImageTransformer2DModel:6 double blocks + 27 single blocks, SwiGLU activations, RoPE,
DistributedAttentionfastvideo/models/encoders/qwen3.py—Qwen3Modeltext encoder(wraps
Ovis2.5-2Bfor conditioning)fastvideo/configs/pipelines/ovis_image.py—OvisImageT2IConfig(flow_shift=3.0, embedded_cfg_scale=5.0, Qwen3 pre/postprocess hooks)
fastvideo/pipelines/basic/ovis_image/—OvisImagePipelinePipeline
fastvideo/pipelines/basic/ovis_image/__init__.pyfastvideo/pipelines/basic/ovis_image/ovis_image_pipeline.pyfastvideo/training/ovis_image_training_pipeline.pyTests
fastvideo/tests/transformers/test_ovisimage.py— transformer forward passfastvideo/tests/encoders/test_qwen3_encoder.py— HF vs FastVideo Qwen3 parityfastvideo/tests/ssim/test_ovis_image_similarity.py— MS-SSIM regression testtests/local_tests/pipelines/test_ovis_image_pipeline_smoke.py— end-to-endVideoGeneratorsmoke testExample
examples/inference/basic/basic_ovis_image.py— runnable exampleFiles modified
fastvideo/registry.py— registeredAIDC-AI/Ovis-Image-7Bfastvideo/configs/models/dits/__init__.py— exportedOvisImageTransformer2DModelConfigfastvideo/configs/models/encoders/__init__.py— exportedQwen3Configfastvideo/configs/models/vaes/base.py— addedload_encoder/load_decoderfieldsfastvideo/models/registry.py— registeredOvisImageTransformer2DModel,Qwen3Modelfastvideo/pipelines/pipeline_registry.py— registeredOvisImagePipelinefastvideo/pipelines/stages/denoising.py—except (ImportError, RuntimeError)for Triton guardsfastvideo/pipelines/stages/causal_denoising.py— same fixfastvideo/pipelines/stages/matrixgame_denoising.py— same fixfastvideo/training/__init__.py— exportedOvisImageTrainingPipelinedocs/inference/support_matrix.md— added Ovis-Image-7B row