Skip to content
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

[WIP] Complete Overhaul...fix EVERY SINGLE relative path. #38

Open
d8ahazard opened this issue Jun 24, 2024 · 14 comments
Open

[WIP] Complete Overhaul...fix EVERY SINGLE relative path. #38

d8ahazard opened this issue Jun 24, 2024 · 14 comments
Assignees
Labels
enhancement New feature or request

Comments

@d8ahazard
Copy link
Contributor

@vinesmsuic - I'm just putting this here so you're not shocked in a few hours...

So, long story short - I want to hook this into ComfyUI and make one custom node to support all the things. Hence the model stuff, hence adding OpenSora1.2 right away, etc.

Even after trying the new version last night, I immediately ran into issue after issue due to trying to import stuff with . notation, versus absolute paths.

As such, I wrote a script that attempts to fix most of the paths, which it did.

Now, I'm about halfway through the process of going through every single python file in the project and doing the following:

  1. Fixing any overlooked relative paths, removing the try/catch stuff.
  2. Removing the path injections. Shouldn't need to do this for a library.
  3. Optimizing imports - for the massive amount of files in the library - this will probably yield a minor performance improvement.
  4. Re-formatting code to PEP8 standards.
  5. Fixing any other issues the original Lib devs had sitting there. For example in Lavie - the original project has a reference to some class that's not even in the code. Probably not a big deal...but just patching stuff like that up should ensure this is a lot more reliable.

Is CogVideo even used? I don't see it imported. It may be the only library that's actually standing in the way of you being able to support windows with this when done.

Also, shall I add SVD support right away too?

@vinesmsuic
Copy link
Member

Hi @d8ahazard , It sounds ambitious and exciting! Really appreciate the motive and the efforts you put in!

The numbered items sound good to me.

  • CogVIdeo is not supported because there was a lot of compatibility issues so we ended up dropping this.
  • Separate PR on SVD support is welcomed.

Let me modify the README.md to add some TODO.

@vinesmsuic vinesmsuic self-assigned this Jun 24, 2024
@vinesmsuic vinesmsuic added enhancement New feature or request help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Jun 24, 2024
@d8ahazard
Copy link
Contributor Author

d8ahazard commented Jun 24, 2024

Hi @d8ahazard , It sounds ambitious and exciting! Really appreciate the motive and the efforts you put in!

The numbered items sound good to me.

  • CogVIdeo is not supported because there was a lot of compatibility issues so we ended up dropping this.
  • Separate PR on SVD support is welcomed.

Let me modify the README.md to add some TODO.

Sounds good. I'll just drop cog entirely then in this refactor, and then once we've validated and tested that, I'll add SVD.

Edit: Also - happy to be a regular contributor on this if you like. I don't have a massive amount of time, but then you can at least assign this to me. If not, no worries either. ;)

@vinesmsuic
Copy link
Member

That sounds like a plan :) Let's do it

@d8ahazard
Copy link
Contributor Author

OK, I've pushed the code (so far) to https://github.com/d8ahazard/VideoGenHub/tree/code_cleanup, testing the load_models script now to ensure that at the very least, all of the pipelines can be loaded.

Then I'll build/install and test locally with ComfyUI. Feel free to start testing, but I'd definitely not publish it yet. :D

@d8ahazard
Copy link
Contributor Author

@vinesmsuic - Still going nuts. I've now created abstract classes for the i2v and t2v models, and started implementing "uniform" functions like load_pipeline, download_models, etc.

Also discovered that OpenSora does support image conditioning, so I created a uniform pipeline for that based on diffusers, will be testing, etc.

@vinesmsuic
Copy link
Member

sounds good! I have plans to add V2V (Video editing) models in the future. Would these uniform functions be compatible?

@d8ahazard
Copy link
Contributor Author

sounds good! I have plans to add V2V (Video editing) models in the future. Would these uniform functions be compatible?

Yes, I think so. We'd basically just add an "input_video" param to the generate_one_video function, and declare a new BaseV2vInferModel class.

Then, we can make InferModels extend from one or more classes, or just behave like T2V if an image/video isn't provided.

@d8ahazard
Copy link
Contributor Author

@vinesmsuic - OK, I think it's ready to test. I've made a LOT of changes, so I'd be shocked if something didn't get broken along the way.

All of the baseInferModels now inherit from either a T2V or I2V base class. I added memory management for every Model, so we don't have to keep reloading the pipeline for multiple video generations, etc.

All of the models now have a download_models function, which, when executed with no params, will download all the models, regardless of possible variants.

They all now have a load_pipeline function too, so if someone wants to use this lib to just expose the various video pipelines, they can do that too.

And, all of the downloading/load pipeline stuff is separate, so we can do things like instantiate the specific Model we want, read the default resolution, prefetch the models, and most importantly, create "global" functions that let us enumerate the T2V, I2V, and all model types and look at their properties without having to load the pipeline, etc.

Feel free to clone and play with it, I want to do a lot more testing before I submit a PR.

@wenhuchen
Copy link
Collaborator

Is there any update on this?

@wenhuchen
Copy link
Collaborator

@d8ahazard ping again to see if there is an update on this.

@vinesmsuic
Copy link
Member

vinesmsuic commented Jul 25, 2024

@d8ahazard I just tried importing every models in the code, seems the new paths are working well! The current code has been merged into the comfy branch. We can start developing the custom nodes?

@d8ahazard
Copy link
Contributor Author

@d8ahazard I just tried importing every models in the code, seems the new paths are working well! The current code has been merged into the comfy branch. We can start developing the custom nodes?

Yes, I think so!

@vinesmsuic
Copy link
Member

@d8ahazard I just tried importing every models in the code, seems the new paths are working well! The current code has been merged into the comfy branch. We can start developing the custom nodes?

Yes, I think so!

Should we create a new repo called ComfyUI-VideoGenHub? It seems a lot of comfyui plugin uses this naming format. Then we can directly import VideoGenHub from the comfy branch in our custom node implementation. Not sure if this is the best way.

@vinesmsuic
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants