-
Notifications
You must be signed in to change notification settings - Fork 26
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
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.
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
examples/pipelines/finetune_stable_diffusion/components/clip_downloader_component/Dockerfile
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/clip_retrieval_component/README.MD
Outdated
Show resolved
Hide resolved
...s/finetune_stable_diffusion/components/clip_retrieval_component/src/utils/embedding_utils.py
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/dataset_loader_component/README.MD
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/dataset_loader_component/src/main.py
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/embedding_component/Dockerfile
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/config/components_config.py
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/dataset_loader_component/src/main.py
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/embedding_component/src/main.py
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/image_filter_component/Dockerfile
Outdated
Show resolved
Hide resolved
examples/pipelines/finetune_stable_diffusion/components/image_filter_component/requirements.txt
Outdated
Show resolved
Hide resolved
|
||
from kfp import components as comp | ||
from kfp import dsl | ||
|
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.
Extra line
@@ -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 |
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.
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 |
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.
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 |
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.
Same comment as above
|
||
|
||
@dataclass | ||
class ClipRetrievalConfig: |
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.
Not needed if we remove clip retrieval from this PR
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.
Thanks for implementing this :)
Refactor stable diffusion pipeline [part 1]
This PR refactors the first 3 components of the Stable Diffusion pipeline by leveraging Express.
The components are: