-
Notifications
You must be signed in to change notification settings - Fork 126
[Diffusion] support Taylorseer on Hunyuan Video #909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for the Hunyuan Video model. The changes include new configuration files for inference and modifications to the inference engine.
My review has identified a few critical issues:
- There are debug
printstatements that should be removed. - A typo
cup_offloadshould becpu_offload, and this needs to be fixed in both the configuration and the code. - There is an unsafe dictionary access that could lead to a
KeyError. - A hardcoded path in an example configuration file makes it non-portable.
Please address these points to improve the code quality and robustness.
| class: diffusers.HunyuanVideoPipeline | ||
| from_pretrained: | ||
| torch_dtype: bfloat16 | ||
| cup_offload: True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ("cup_offload" in kwargs["pipeline"]["from_pretrained"] and kwargs["pipeline"]["from_pretrained"]["cup_offload"]): | ||
| pipeline.enable_model_cpu_offload() | ||
| else: | ||
| device = kwargs.get("device", None) | ||
| if device: | ||
| pipeline.to(device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block has two issues:
- Typo:
cup_offloadshould becpu_offload. This is to correctly triggerpipeline.enable_model_cpu_offload(). - Unsafe Dictionary Access: Accessing
kwargs["pipeline"]["from_pretrained"]directly is unsafe and will raise aKeyErrorifpipelineorfrom_pretrainedkeys are missing in the configuration. You should use.get()for safe access.
Please correct the typo and refactor for safe dictionary access.
| if ("cup_offload" in kwargs["pipeline"]["from_pretrained"] and kwargs["pipeline"]["from_pretrained"]["cup_offload"]): | |
| pipeline.enable_model_cpu_offload() | |
| else: | |
| device = kwargs.get("device", None) | |
| if device: | |
| pipeline.to(device) | |
| from_pretrained_cfg = kwargs.get("pipeline", {}).get("from_pretrained", {}) | |
| if from_pretrained_cfg.get("cpu_offload"): | |
| pipeline.enable_model_cpu_offload() | |
| else: | |
| device = kwargs.get("device", None) | |
| if device: | |
| pipeline.to(device) |
| runner: | ||
| hostfile: null | ||
| cmds: | ||
| before_start: source /root/miniconda3/bin/activate flagscale-inference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if v is not None and k not in ("model", "loader") | ||
| } | ||
|
|
||
| print(f"self.vconfig:{self.vconfig}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| start_time = time.time() | ||
| outputs = self.model_or_pipeline(**kwargs) | ||
| gen_time = time.time() - start_time | ||
| print(f"gen_time: {gen_time:.2f}s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines for timing and printing the generation time appear to be for debugging. Such print statements should be removed from the final code. If performance measurement is a desired feature, it should be implemented using a proper logging framework (like the logger already imported in this file) instead of printing to stdout.
| start_time = time.time() | |
| outputs = self.model_or_pipeline(**kwargs) | |
| gen_time = time.time() - start_time | |
| print(f"gen_time: {gen_time:.2f}s") | |
| outputs = self.model_or_pipeline(**kwargs) |
PR Category
Inference
PR Types
User Experience
PR Description