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

Refactor stable diffusion pipeline [part 1] #20

Merged
merged 38 commits into from
Apr 4, 2023

Conversation

NielsRogge
Copy link
Contributor

@NielsRogge NielsRogge commented Mar 28, 2023

This PR refactors the first 3 components of the Stable Diffusion pipeline by leveraging Express.

The components are:

  1. dataset downloader component (loads a dataset from the hub, creates initial manifest)
  2. image filter component (filters images based on requested width and height, only updates the index)
  3. embedding component (uses CLIP to embed the filtered images, adds a new data source to the manifest)

Copy link
Contributor

@PhilippeMoussalli PhilippeMoussalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments. Looks good to me overall, the components seem much neater and compact with express compared to the previous implementation 😃

General comments:

  • Only delete the components that are replaced and keep the old ones as reference. Also keep the docs explaining what the components do (you can modify them as needed)
  • Make sure the requirements are fixed for all components
  • Change the base image for all docker images that use GPU/Torch to one of the official pytorch images

I think we should also add the folders that are been changed to the pre-commit configs to lint them. See an example on how this is done here:

https://github.com/ml6team/express/pull/6/files

For now, you can point to every individual component that you modify and then later on we can change to point to pipelines/finetune-stable-diffusion


from kfp import components as comp
from kfp import dsl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line

@NielsRogge NielsRogge marked this pull request as ready for review March 30, 2023 07:44
@NielsRogge NielsRogge changed the title [WIP] Refactor stable diffusion pipeline Refactor stable diffusion pipeline [part 1] Mar 30, 2023
@@ -1,14 +1,31 @@
FROM europe-west1-docker.pkg.dev/storied-landing-366912/storied-landing-366912-default-repository/mlpipelines/kubeflow/components/base_component:latest
FROM --platform=linux/amd64 python:3.8-slim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove --platform=linux/amd64 and only have it as a flag in the bash script?

@@ -1,58 +1,29 @@
name: clip_retrieval_component
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove all clip retrieval related stuff from this PR?

@@ -0,0 +1,29 @@
FROM --platform=linux/amd64 pytorch/pytorch:2.0.0-cuda11.7-cudnn8-runtime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above



@dataclass
class ClipRetrievalConfig:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed if we remove clip retrieval from this PR

Copy link
Contributor

@PhilippeMoussalli PhilippeMoussalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing this :)

@NielsRogge NielsRogge merged commit 01f4022 into main Apr 4, 2023
@RobbeSneyders RobbeSneyders deleted the refactor_stable_diffusion_backup branch May 15, 2023 16:29
Hakimovich99 pushed a commit that referenced this pull request Oct 16, 2023
Refactor stable diffusion pipeline [part 1]
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.

2 participants