integrate MOVA model with LightX2V (pre-alpha, requires manual path adjustment)#898
integrate MOVA model with LightX2V (pre-alpha, requires manual path adjustment)#898zaochen123 wants to merge 1 commit intoModelTC:mainfrom
Conversation
Summary of ChangesHello @zaochen123, 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 introduces preliminary support for the MOVA model within the LightX2V framework. The integration focuses on enabling image-to-audio-video generation by wrapping the official MOVA pipeline, allowing it to benefit from LightX2V's performance enhancements. This provides users with the capability to generate high-quality videos using the MOVA model, albeit with an initial manual path configuration step. 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
Activity
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 pull request introduces initial support for the MOVA model. While this is a valuable addition, the current implementation has several critical issues that should be addressed before merging, even for a pre-alpha version. The code contains numerous hard-coded absolute paths, making it non-portable. The example script is not runnable without significant manual changes. The MOVARunner integration with the existing framework is not clean, bypassing the intended abstractions and making it difficult to maintain. There are also potential resource leaks and robustness issues, such as an un-deleted temporary file and a hard-coded network port. My review provides specific suggestions to resolve these problems.
| { | ||
| "model_cls": "mova", | ||
| "task": "i2av", | ||
| "model_path": "/devsft_AFS/mova_weights", |
There was a problem hiding this comment.
Hard-coded absolute paths should be avoided as they make the configuration not portable. Please use a placeholder value like <PATH_TO_YOUR_MOVA_WEIGHTS> and add a note in the documentation for users to replace it. This makes it explicit that a user action is required.
| "model_path": "/devsft_AFS/mova_weights", | |
| "model_path": "<PATH_TO_YOUR_MOVA_WEIGHTS>", |
| @@ -0,0 +1,36 @@ | |||
| import os | |||
| import sys | |||
| sys.path.append('/devsft_AFS/mova_code/LightX2V') | |||
There was a problem hiding this comment.
Modifying sys.path with a hard-coded absolute path is a critical anti-pattern. It makes the script entirely non-portable and dependent on a specific developer's environment. This should be removed. The project should be set up to be runnable without such hacks, for example by installing it in editable mode (pip install -e .) and running examples from the project root.
| pipe = LightX2VPipeline( | ||
| model_path="/devsft_AFS/mova_weights", # 权重路径 | ||
| model_cls="mova", | ||
| task="i2av", | ||
| ) |
There was a problem hiding this comment.
This script contains multiple hard-coded absolute paths for model, config (lines 8, 13), and output files (lines 27, 28). This makes the example impossible to run for other users without manually editing the source code. These paths should be parameterized using command-line arguments (e.g., with argparse). Default values can be relative paths to make the example easier to run out-of-the-box.
|
|
||
|
|
||
| @RUNNER_REGISTER("mova") | ||
| class MOVARunner(DefaultRunner): |
There was a problem hiding this comment.
The MOVARunner inherits from DefaultRunner but doesn't adhere to its contract. It overrides run_pipeline with a completely custom implementation and leaves other essential methods like run_text_encoder and run_vae_encoder as empty placeholders. This breaks the abstraction and makes the code harder to maintain and reason about. Consider refactoring this to either properly implement the DefaultRunner interface or inherit from BaseRunner if the execution model is fundamentally different.
| temp_file = tempfile.NamedTemporaryFile(delete=False) | ||
| init_method = f"file://{temp_file.name}" | ||
| temp_file.close() | ||
| dist.init_process_group( | ||
| backend="nccl", | ||
| init_method=init_method, | ||
| rank=0, | ||
| world_size=1 | ||
| ) | ||
| print("Initialized single-process distributed group with file init_method.") | ||
| self._dist_initialized = True | ||
| return True |
There was a problem hiding this comment.
The temporary file created for the file-system-based process group initialization is not being deleted, which will lead to a resource leak, leaving temporary files on the system. You should wrap the initialization logic in a try...finally block to ensure the file is removed after it's no longer needed.
| temp_file = tempfile.NamedTemporaryFile(delete=False) | |
| init_method = f"file://{temp_file.name}" | |
| temp_file.close() | |
| dist.init_process_group( | |
| backend="nccl", | |
| init_method=init_method, | |
| rank=0, | |
| world_size=1 | |
| ) | |
| print("Initialized single-process distributed group with file init_method.") | |
| self._dist_initialized = True | |
| return True | |
| temp_file = tempfile.NamedTemporaryFile(delete=False) | |
| try: | |
| init_method = f"file://{temp_file.name}" | |
| temp_file.close() | |
| dist.init_process_group( | |
| backend="nccl", | |
| init_method=init_method, | |
| rank=0, | |
| world_size=1 | |
| ) | |
| print("Initialized single-process distributed group with file init_method.") | |
| self._dist_initialized = True | |
| return True | |
| finally: | |
| if os.path.exists(temp_file.name): | |
| os.unlink(temp_file.name) |
| if not self._dist_initialized: | ||
| if not self._init_distributed(): | ||
| os.environ.setdefault("MASTER_ADDR", "localhost") | ||
| os.environ.setdefault("MASTER_PORT", "29500") |
There was a problem hiding this comment.
Hard-coding the MASTER_PORT to 29500 can lead to port conflicts if multiple processes are run on the same machine. It's more robust to find a free port dynamically at runtime, for example by using the socket module to bind to port 0 and get the assigned port.
zaochen123
force-pushed
the
feat/mova-integration
branch
from
b3135fd to
65e6e22
Compare
2 participants
This PR adds initial support for the MOVA model (OpenMOSS/MOVA) to LightX2V. The integration directly calls the official MOVA pipeline, ensuring correctness while enabling LightX2V’s base optimizations (FlashAttention, etc.).
The code currently contains hard‑coded absolute paths pointing to the developer’s AFS environment (
/devsft_AFS/). Before running, you must replace these with your own paths. The following files need adjustment:configs/mova/mova_t2v.json"model_path": change to your local MOVA weights directory.examples/mova/mova_t2v.pyconfig_path: update to the correct location of the config file.image_path: set to your input image.save_result_path: set to your desired output location.Example of required changes (search for
/devsft_AFSin the files).What’s included
lightx2v/models/runners/mova/mova_runner.pyexamples/mova/mova_t2v.pyconfigs/mova/mova_t2v.jsonHow to test after fixing paths
pip install -e /path/to/mova_officialcd examples/mova python mova_t2v.py