Skip to content

Conversation

@Wauplin
Copy link
Collaborator

@Wauplin Wauplin commented Mar 31, 2023

Related to #2860.

Updated some training scripts to use upload_folder instead of Repository.push_to_hub. The two advantages are upload speed (x1.5?) and UX (progress bars).

As said in #2860 (comment), upload_folder does not support blocking=False (see huggingface/huggingface_hub#939). This means I did not update the scripts that are pushing weights while training (train_dreambooth_flax.py, ./research_projects/onnxruntime/unconditional_image_generation/train_unconditional.py and ./unconditional_image_generation/train_unconditional.py).

Worth mentioning that this is not ISO-feature with the previous scripts. In particular, upload_folder does not take into account if there is a .gitignore file in the output_dir already.

(Personal opinion: I think this PR makes the training script a bit cleaner (less duplicated code) but if you prefer to stick to the existing ones for consistency, I completely get it as well)

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Apr 3, 2023

Can we merge this one in?
I tested the Pytorch controlnet here https://huggingface.co/YiYiXu/test_uploadfolder
is it sufficient? or do we have to test all the scripts?

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks a lot for handling this one!

Also, I love the little cleaning you have done to remove the bloating of code.

Could we also ensure the related tests pass the CI?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 3, 2023

The documentation is not available anymore as the PR was closed or merged.

@sayakpaul
Copy link
Member

@yiyixuxu could you give this a quick review as well?

I think we're good to merge this one.

Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

thanks!

@sayakpaul
Copy link
Member

@patrickvonplaten could you give it a quick look as well?

@Wauplin
Copy link
Collaborator Author

Wauplin commented Apr 4, 2023

Some tests are still failing but I'm not sure they are related to my changes.

Otherwise, I wrote a few warnings in the description but that's it from my side.

@sayakpaul
Copy link
Member

Yes, sorry @Wauplin!

https://github.com/huggingface/diffusers/actions/runs/4599394852/jobs/8124713689?pr=2934 should be fixed with #2959. MPS test failure is unrelated.

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Super cool PR - much more concise thanks a lot!

@patrickvonplaten
Copy link
Contributor

Merging, taking:

I think we're good to merge this one.

as a "approved"

@patrickvonplaten patrickvonplaten merged commit a87e88b into main Apr 4, 2023
w4ffl35 pushed a commit to w4ffl35/diffusers that referenced this pull request Apr 14, 2023
use upload folder in training scripts

Co-authored-by: testbot <lucainp@hf.co>
dg845 pushed a commit to dg845/diffusers that referenced this pull request May 6, 2023
use upload folder in training scripts

Co-authored-by: testbot <lucainp@hf.co>
@kashif kashif deleted the 2860-use-upload-folder-in-training-scripts branch June 7, 2023 09:41
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
use upload folder in training scripts

Co-authored-by: testbot <lucainp@hf.co>
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.

6 participants