-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add a shortfin pipeline for flux #876
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.
I was not able to review all the code. I will continue later.
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.
The main concern I have is there is a lot of code duplicated from the SDXL pipeline.
I am not sure when it would be best to address this, but we definitely need to address this at some point. When eyeballing this PR I think there is more than 1K lines of code duplicated.
I still have a bit left to review.
shortfin/python/shortfin_apps/flux/components/config_artifacts.py
Outdated
Show resolved
Hide resolved
shortfin/python/shortfin_apps/flux/components/config_artifacts.py
Outdated
Show resolved
Hide resolved
I agree that the code duplication is a major issue. There's massive duplication across flux/sdxl/llama pipelines and it makes me really uncomfortable to be duplicating this much but I was handed a mostly complete shortfin flux pipeline from Ean and I don't have as much expertise to make sensible abstractions and reutilizations. It might be a major effort to do a good refactoring across these shortfin pipelines, but if you think its the right move to do it now, I'll work on that. |
We already have a functional Flux pipeline in this PR. I will not block this to move things forward, but I think after that we need to refactor. It would be nice to involve the people who wrote the SDXL and Llama pipelines as they may have valuable design ideas. |
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.
I did not see an integration test of the whole pipeline.
I think this is a next step and once it is under test we can focus on refactoring all the pipelines.
Co-authored-by: Ean Garvey <ean.garvey@amd.com>
No description provided.