[do not merge] Rebased refactor #270

jzhang38 · 2025-03-16T23:49:11Z

[ ] Attn backend (PY)
[ ] Wan text encoder (PY)
[ ] wan vae (Wei)
[ ] wan pipeline (PY & Wei)
[ ] Merge wan dit code to refactor (Will)
[ ] Clean up code & loader directory（Will)
[ ] hunyuan text encoder (Will)

Co-authored-by: Will Lin <[email protected]>

Co-authored-by: Peiyuan Zhang <[email protected]>

Signed-off-by: <> Co-authored-by: Will Lin <[email protected]> Co-authored-by: Ubuntu <ubuntu@awesome-gpu-name-8-inst-2tbsnfodvpomxv4tukw2dkfgyvz.c.nv-brev-20240723.internal> Co-authored-by: Ubuntu <ubuntu@awesome-gpu-name-9-inst-2tpydiudxfu1jg9xvpflm7oexie.c.nv-brev-20240723.internal>

…/FastVideo into rebased-refactor

Add wan dit

comaniac · 2025-03-20T18:48:38Z

fastvideo/v1/models/schedulers/scheduling_flow_match_euler_discrete.py

+# Copyright 2024 Stability AI, Katherine Crowson and The HuggingFace Team. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+#
+# Modified from diffusers==0.29.2
+#
+# ==============================================================================


Suggestion for licensing:

All files need Apache license headers. It is recommended to use a simplified one-liner:

# SPDX-License-Identifier: Apache-2.0

For the files copied from other projects, if that project is in Apache-2.0 as well (e.g. vLLM/SGLang), then just

# SPDX-License-Identifier: Apache-2.0 # Adapted from: <link to the original file>

For the files copied from other projects, if that project is in other licenses (e.g. Copyright), you need to read their copyright and make sure you can copy it. If so, then the header could be

# Copyright 2024 Stability AI, Katherine Crowson and The HuggingFace Team. All rights reserved. # SPDX-License-Identifier: Apache-2.0 # Adapted from: <link to the original file>

One way to eliminate the full license in every files you copied, you could have license files for each project, such as https://github.com/awslabs/raf/tree/main/licenses

comaniac · 2025-03-20T18:58:10Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        modules["text_encoder"] = text_encoder
+        modules["text_encoder_2"] = text_encoder_2


Where are those modules used and how these keys (e.g., text_encoder_2) are determined? A more general question is if a model provider wants to implement a pipeline, does it trivial for them to know how should they implement this function?

Same question I think as my above one

comaniac · 2025-03-20T19:01:34Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        self.add_stage("decoding_stage", 
+                       DecodingStage())
+
+    def initialize_pipeline(self, inference_args: InferenceArgs):


Ditto. It seems not clear about what to initialize in this function.

comaniac · 2025-03-20T19:03:56Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        # for stage in self._stages:
+            # batch = stage(batch, inference_args)


Definitely prefer this programming model; otherwise it's meaningless to have stages.

If users want to custom logic between stages, you should ask them to implement a custom stage (so ofc the API of wrapping custom logic to be a pipeline stage has to be clean and simple), and add that stage to the pipeline; otherwise it's hard to apply pipeline-level optimizations because you may not have the fully picture.

comaniac · 2025-03-20T19:05:03Z

fastvideo/v1/pipelines/stages/base.py

+        Returns:
+            The updated batch information after this stage's processing.
+        """
+        pass


Should raise NotImplementedError?

Also the name _call_implementation seems not clear and concise enough. Maybe just call() (it's better to make this a public API without the underline prefix)?

comaniac · 2025-03-20T19:08:01Z

fastvideo/v1/pipelines/stages/base.py

+            # Just call the implementation directly if logging is disabled
+            return self._call_implementation(batch, inference_args)


For better user experience it's better to validate input and output batch (e.g., check expected keys) before and after calling the implementation.

comaniac · 2025-03-20T19:42:03Z

fastvideo/v1/pipelines/stages/denoising.py

+        prompt_embeds_2 = batch.prompt_embeds_2
+
+        # Run denoising loop
+        with self.progress_bar(total=num_inference_steps) as progress_bar:


We should not handle display stuffs in this function.

comaniac · 2025-03-20T19:45:05Z

fastvideo/v1/pipelines/stages/denoising.py

+                    # TODO(will): finalize the interface
+                    with set_forward_context(num_step=i, inference_args=inference_args):
+                        # Run transformer
+                        noise_pred = self.transformer(


This pattern is confusing, because it means this stage cannot be used without pipeline. Ideally every stage should be self-contained. For example

def __init__(self, ...): self.transformers = ... def _call_implementation(self, ...): batch = self.transformers(...)

Co-authored-by: kevin314 <[email protected]>

zhisbug · 2025-03-21T20:33:38Z

scripts/inference/v1_inference_hunyuan.sh

inference_hunyuan_v1 instead of v1_inference_hunyuan?

zhisbug · 2025-03-25T08:09:58Z

predict.py

@@ -12,7 +12,7 @@
 import torchvision
 from cog import BasePredictor, Input, Path
 from einops import rearrange
-
+from transformers import LlamaForCausalLM


nit: I think you indeed need a space to make import three sections?

please run linting and code style formatter

I think we can delete this file? @SolitaryThinker

zhisbug · 2025-03-25T08:16:17Z

predict.py

do we still need a separate predction.py?.

First, this file feels like a bit out of place.

Second, remember we are pursuing a cli like interface like fastvideo generate --args

zhisbug · 2025-03-25T08:17:14Z

env_setup.sh

nit: can we just put dependency in toml and remove this file

zhisbug · 2025-03-25T08:28:00Z

fastvideo/v1/attention/backends/abstract.py

@@ -0,0 +1,213 @@
+# SPDX-License-Identifier: Apache-2.0


not sure if you have done this, but if this is copied/adapted from vLLM, let's mark the source file in the header?

zhisbug · 2025-03-25T23:33:22Z

fastvideo/v1/models/loader/component_loader.py

+    def load(self, model_path: str, architecture: str, inference_args: InferenceArgs):
+        """Load the scheduler based on the model path, architecture, and inference args."""
+
+        scheduler = get_scheduler(


just educate me: what do we need to load for scheduler?

zhisbug · 2025-03-25T23:50:40Z

fastvideo/v1/models/text_encoder.py

I am confused about the encoder design here. We have a models/encoder folder where we put different encoder models, yet we have a separate text_encoder.py. What are their boundaries?

zhisbug · 2025-03-26T00:04:09Z

fastvideo/v1/pipelines/composed/composed_pipeline_base.py

so, why we need a composed package? This package is rather thin. Seems to only provide a abstraction on the ComposePipeline object?

maybe a simple structure is just like:

pipllines/ -- composed_pipeline_base.py -- hunyuan_pipeline.py -- pipeline_regstry.py -- stages/ ...?

zhisbug · 2025-03-26T00:08:25Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        # prompt_template
+        prompt_template = PROMPT_TEMPLATE["image"]
+
+        # prompt_template_video


feel the constant.py is unnecessary; could just put everything into one hunyuan_pipeline.py

+1, i think it's not good to split

zhisbug · 2025-03-26T00:09:46Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+
+        # or 
+
+        batch = self.input_validation_stage(batch, inference_args)


agree with @comaniac. I am confused here -- in the above you already have clearly defined stages? so the block block below could just be discarded?

richardliaw

Agree with others comment about avoiding the add_stage API compared to something more fluent

richardliaw · 2025-03-26T00:53:35Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+
+class HunyuanVideoPipeline(ComposedPipelineBase):
+
+    def required_config_modules(self):


Why is this a func and not a property or a class var?

richardliaw · 2025-03-26T00:54:04Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+    def initialize_encoders(self, inference_args: InferenceArgs):
+        self.initialize_encoders_v1(inference_args)


Seems like boilerplate?

richardliaw · 2025-03-26T00:54:56Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        # prompt_template
+        prompt_template = PROMPT_TEMPLATE["image"]
+
+        # prompt_template_video


+1, i think it's not good to split

richardliaw · 2025-03-26T00:56:36Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        encoder_1.to(dtype=PRECISION_TO_TYPE[inference_args.text_encoder_precision])
+        encoder_1.requires_grad_(False)
+
+        print(f"keys: {self.modules.keys()}")


richardliaw · 2025-03-26T00:57:05Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+            device=inference_args.device if not inference_args.use_cpu_offload else "cpu",
+        )
+
+        encoder_2 = self.modules.pop("text_encoder_2")


why do you have to pop

richardliaw · 2025-03-26T00:59:55Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        text_encoder_2 = TextEncoder(
+            text_encoder=encoder_2,
+            tokenizer=tokenizer_2,
+            # text_encoder_type="text_encoder_2",
+            max_length=inference_args.text_len_2,
+            # text_encoder_precision=inference_args.text_encoder_precision,
+            device=inference_args.device if not inference_args.use_cpu_offload else "cpu",
+        )
+        self.modules["text_encoder"] = text_encoder
+        self.modules["text_encoder_2"] = text_encoder_2


Why do you do this pop and set thing? Feels like you should have something more functional that doesn't make the user do all of this boilerplate -- like you set device in 2 places, requires_grad_ and finding the tokenizer is basically boilerplate

richardliaw · 2025-03-26T01:00:15Z

fastvideo/v1/pipelines/implementations/hunyuan/hunyuan_pipeline.py

+        modules["text_encoder"] = text_encoder
+        modules["text_encoder_2"] = text_encoder_2


Same question I think as my above one

SolitaryThinker and others added 30 commits March 14, 2025 19:07

Initial set of common files and layers from vLLM (#226)

42f902c

add sp comm (#231)

ac07e43

Initial clip encoder and cli args organization (#232)

5252d50

[Refactor] Add Hunyuan DiT Modeling (#241)

0f4c8d1

Refactor py (#246)

fc5a4bc

v1 staging architecture

6e8b11c

Co-authored-by: Will Lin <[email protected]>

[Do not merge] V1 encoders and model loading (#261)

23fd3ed

Co-authored-by: Peiyuan Zhang <[email protected]>

DiT done and plub in pipeline (#252)

b2def4b

move refactor to fastvideo/v1 (#265)

b631546

remove unneeded file

bf1fc27

revert pyproject.toml

eafeea4

move v1's v0 code into v1/v0_reference_src

1976b23

remove unused attention

d5ac1e9

fix import paths

d2db0d4

Add wan dit

e976583

debugging encoders

fb44fba

fix attn

759f243

running, correctness isues

f51e9d4

add toggle flags for v0 pipeline components

796eaf8

vae update

691f9d1

update

83348c6

Merge branch 'rebased-refactor' of https://github.com/SolitaryThinker…

4e056b9

…/FastVideo into rebased-refactor

magic line

c37535a

update

ae5ed0c

Merge pull request #1 from SolitaryThinker/wei

6c52846

Add wan dit

model/loader.py -> component_loader.py

e14d384

moved loader/ into models/

bb6ae36

cleanup

8140cb2

cleanup

dd10588

This was referenced Mar 16, 2025

Refactor #244

Closed

[do not merge] Rebased refactor branch #266

Closed

jzhang38 changed the title ~~Rebased refactor~~ [do not merge] Rebased refactor Mar 16, 2025

jzhang38 marked this pull request as draft March 16, 2025 23:50

jzhang38 and others added 3 commits March 16, 2025 19:23

Streamline pipeline (#271)

dc2a451

[v1] cleanup and remove more vllm dependencies (#274)

2f59409

[v1] Attention backend abstraction and Flash-attn backend (#276)

31057b6

comaniac reviewed Mar 20, 2025

View reviewed changes

SolitaryThinker and others added 2 commits March 24, 2025 23:04

refactor pipeline (#281)

bc2c8df

Add CLI for inference (#277)

86d2786

Co-authored-by: kevin314 <[email protected]>

zhisbug requested changes Mar 26, 2025

View reviewed changes

richardliaw reviewed Mar 26, 2025

View reviewed changes

SolitaryThinker added 6 commits March 25, 2025 22:29

revert predict.py

aa0ac8b

format 80 column

f8890c7

format config

42ad1e7

removed composed package in pipeline

5aeb825

remove xpu/hip

37ffba7

remove more code

ce56abb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[do not merge] Rebased refactor #270

[do not merge] Rebased refactor #270

jzhang38 commented Mar 16, 2025 •

edited

Loading

comaniac Mar 20, 2025

comaniac Mar 20, 2025

richardliaw Mar 26, 2025

comaniac Mar 20, 2025

comaniac Mar 20, 2025

comaniac Mar 20, 2025

comaniac Mar 20, 2025

comaniac Mar 20, 2025

comaniac Mar 20, 2025

zhisbug Mar 21, 2025

zhisbug Mar 25, 2025

zhisbug Mar 25, 2025

jzhang38 Mar 26, 2025

zhisbug Mar 25, 2025

zhisbug Mar 25, 2025

zhisbug Mar 25, 2025

zhisbug Mar 25, 2025

zhisbug Mar 25, 2025

zhisbug Mar 26, 2025

zhisbug Mar 26, 2025

zhisbug Mar 26, 2025

richardliaw Mar 26, 2025

zhisbug Mar 26, 2025

richardliaw left a comment

richardliaw Mar 26, 2025

richardliaw Mar 26, 2025

richardliaw Mar 26, 2025

richardliaw Mar 26, 2025

richardliaw Mar 26, 2025

richardliaw Mar 26, 2025

richardliaw Mar 26, 2025

		modules["text_encoder"] = text_encoder
		modules["text_encoder_2"] = text_encoder_2

		# for stage in self._stages:
		# batch = stage(batch, inference_args)

		# Just call the implementation directly if logging is disabled
		return self._call_implementation(batch, inference_args)


		# or

		batch = self.input_validation_stage(batch, inference_args)


		class HunyuanVideoPipeline(ComposedPipelineBase):

		def required_config_modules(self):

		def initialize_encoders(self, inference_args: InferenceArgs):
		self.initialize_encoders_v1(inference_args)

[do not merge] Rebased refactor #270

Are you sure you want to change the base?

[do not merge] Rebased refactor #270

Conversation

jzhang38 commented Mar 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardliaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzhang38 commented Mar 16, 2025 •

edited

Loading