-
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 into library structure #2
Conversation
...finetune_stable_diffusion/components/image_classifier_component/src/classifiers/clean_cut.py
Outdated
Show resolved
Hide resolved
examples/finetune_stable_diffusion/pipelines/dataset_creation_pipeline.py
Outdated
Show resolved
Hide resolved
Could you re-add the dependencies from the removed |
@@ -1,83 +0,0 @@ | |||
"""Clean cut classifier""" |
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.
Seems like this file can still be fetched from the history. Might want to find a way to remove it completely
@@ -51,4 +51,4 @@ exclude_dirs = ["*/mlpipelines/components/sd_finetuning_component/src/train_text | |||
|
|||
[build-system] | |||
requires = ["poetry-core>=1.2.0"] | |||
build-backend = "poetry.core.masonry.api" | |||
build-backend = "poetry.core.masonry.api" |
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 you make sure that express
(or express_components
) is installable and can be used. I think this is missing a packages
field. @RobbeSneyders is the plan still to have the setup.py
in the common
folder or should all the epxress dependencies move to here?
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 read a bit the docs of poetry and it turns out there's no need for a packages
field, if the name
field has the same name as the package. I've confirmed that I can install the express package using poetry install
.
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.
All dependencies should move here indeed.
We shouldn't need a packages
field, Poetry should detect it automatically.
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.
Made some final changes myself. Will already merge so we can continue the work in parallel.
This PR refactors the overall design of Express. Main changes include: - include the main functionality of Express (helper functions, LoaderComponent and TransformComponent classes) in the "express" folder - include the Stable Diffusion components in an "examples" folder - include a "docs" folder where all documentation lives To do: - [ ] add a "tests" folder where all tests live
This PR refactors the overall design of Express. Main changes include: - include the main functionality of Express (helper functions, LoaderComponent and TransformComponent classes) in the "express" folder - include the Stable Diffusion components in an "examples" folder - include a "docs" folder where all documentation lives To do: - [ ] add a "tests" folder where all tests live
This PR refactors the overall design of Express.
Main changes include:
To do: