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

Add a shortfin pipeline for flux #876

Merged
merged 32 commits into from
Feb 17, 2025
Merged

Add a shortfin pipeline for flux #876

merged 32 commits into from
Feb 17, 2025

Conversation

KyleHerndon
Copy link
Contributor

No description provided.

Copy link
Contributor

@sogartar sogartar left a 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.

Copy link
Contributor

@sogartar sogartar left a 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.

@KyleHerndon
Copy link
Contributor Author

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.

@sogartar
Copy link
Contributor

sogartar commented Feb 11, 2025

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.

Copy link
Contributor

@sogartar sogartar left a 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.

@KyleHerndon KyleHerndon merged commit ab42f0c into main Feb 17, 2025
37 of 38 checks passed
@KyleHerndon KyleHerndon deleted the kyle-flux branch February 17, 2025 06:00
renxida pushed a commit to renxida/shark-ai that referenced this pull request Feb 20, 2025
Co-authored-by: Ean Garvey <ean.garvey@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants