Skip to content

Conversation

@davidberenstein1957
Copy link
Contributor

@davidberenstein1957 davidberenstein1957 commented Dec 10, 2024

What does this PR do?

  • fix to force casting and image column to decode to ensure this is done for feature sets where this is not the default
  • fix to allow for not passing an instance_prompt

Fixes # NA

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Copy link
Contributor Author

@davidberenstein1957 davidberenstein1957 left a comment

Choose a reason for hiding this comment

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

cc @linoytsaban @sayakpaul, minor fixes. I can create separate issue to link them to if preferred.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@linoytsaban
Copy link
Collaborator

Thanks @davidberenstein1957! re: instance_prompt we fall back to it in case captions are not provided / there's an issue with provided captions, so I think it's safer to keep it. Is there some motivation I'm missing for removing the requirement?

raise ValueError(
f"`--image_column` value '{args.image_column}' not found in dataset columns. Dataset columns are: {', '.join(column_names)}"
)
dataset["train"] = dataset["train"].cast_column(image_column, Image(decode=True))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

@davidberenstein1957 davidberenstein1957 Dec 11, 2024

Choose a reason for hiding this comment

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

@sayakpaul fix to force casting and image column to decode to ensure this is done for feature sets where decoding automatically is not the default

Copy link
Member

Choose a reason for hiding this comment

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

Okay let's have that as a comment too. Thanks for explaining!

@sayakpaul
Copy link
Member

re: instance_prompt we fall back to it in case captions are not provided / there's an issue with provided captions, so I think it's safer to keep it. Is there some motivation I'm missing for removing the requirement?

I think okay if it's made a non-required argument, though.

@davidberenstein1957
Copy link
Contributor Author

@sayakpaul @linoytsaban I just realised these fixes might also need to be applied to other dreambooth scripts?

@linoytsaban
Copy link
Collaborator

I just realised these fixes might also need to be applied to other dreambooth scripts?

@davidberenstein1957 yes :) this can apply to -

  • dreambooth/train_dreambooth_lora_sdxl.py
  • dreambooth/train_dreambooth_lora_sd3.py
  • dreambooth/train_dreambooth_flux.py
  • dreambooth/train_dreambooth_sdxl.py
  • dreambooth/train_dreambooth_sd3.py
  • advanced_diffusion_training/train_dreambooth_lora_sdxl_advanced.py
  • advanced_diffusion_training/train_dreambooth_lora_flux_advanced.py
  • advanced_diffusion_training/train_dreambooth_lora_sd15_advanced.py

@linoytsaban
Copy link
Collaborator

I think okay if it's made a non-required argument, though.

@sayakpaul @davidberenstein1957 I'm not opposed to it either, but it might be good to add a informative warning/throw an error in places we relied on instance_prompt being provided

@davidberenstein1957
Copy link
Contributor Author

@linoytsaban will make sure to add some context if neither one of the required params is provided.

@sayakpaul
Copy link
Member

Error sounds good. We could tackle this just for a single script and open the rest to the community. I don't think we need to be exhaustive at all.

@linoytsaban
Copy link
Collaborator

Error sounds good. We could tackle this just for a single script and open the rest to the community. I don't think we need to be exhaustive at all.

Yeah I agree

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Jan 10, 2025
@linoytsaban linoytsaban removed the stale Issues that haven't received updates label Jan 14, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2025

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Issues that haven't received updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants