Textual inv make save log both steps #2178

isamu-isozaki · 2023-01-31T17:12:05Z

Based on this issue. Still a WIP. I'm trying to keep the logging functionality to be the same as the same functionality where every x amounts of steps, we log.

I was thinking of moving the logic of the logging to a function. But then I need a way to get wandb there. I was thinking of passing it in as a default parameter or if available, automatically import. Let me know if anyone has some ideas for this.

HuggingFaceDocBuilderDev · 2023-01-31T17:16:36Z

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

isamu-isozaki · 2023-01-31T19:34:27Z

will try fixing the checks. Bit confused why it failed though.

patil-suraj · 2023-02-07T11:12:42Z

examples/textual_inversion/textual_inversion.py

@@ -270,11 +270,11 @@ def parse_args():
        help="Number of images that should be generated during validation with `validation_prompt`.",
    )
    parser.add_argument(
-        "--validation_epochs",
+        "--validation_steps",


Using steps here (and in dreambooth) script makes sense to me and also it aligns well with save_steps. But Removing the existing arg would be a breaking change,

Instead of changing the arg, maybe we could introduce an additional arg called validation_steps and allow only one of them to be set. Inside the script, we always use validation_steps , we can compute that easily using the epochs value. And to align saving and validating we could prefer the validation_steps argument in the docs.

wdyt @patrickvonplaten @williamberman @pcuenca ?

@patil-suraj good point! One point that I have is to do this we might want to make the logging a function(because calling in 2 places)

But if we do that we need to pass wandb as an argument as we aren't doing the import in the global scope. I was thinking of doing this before but it felt a bit hacky. Happy to show what I mean!

Preferring config via steps over epochs makes sense to me 👍

Steps also makes sense to me but let's deprecate validation_epochs nicely. Could we keep validation_epochs and just add a new validation_steps . Then will convert validation_epochs to validation_steps if used and throw a deprecation warning. Would that be ok for you @isamu-isozaki ?

@patrickvonplaten yup sounds good! Then will move the code to a function so it's cleaner

@patrickvonplaten Should be done!

williamberman · 2023-02-07T20:26:19Z

@isamu-isozaki I'm not sure I see what logic would have to be moved to a function? Regardless, I do think importing wandb on the top level of the module is better as I don't think there's intention to scope the import to the function.

isamu-isozaki · 2023-02-07T21:44:10Z

@williamberman Thanks for the review! Ah if we just going with steps then no worries! I was mainly talking about if we want to support logging either for steps or epochs

isamu-isozaki · 2023-02-07T21:45:27Z

Will fix merge conflict

patil-suraj · 2023-02-08T08:49:46Z

Hey @isamu-isozaki we just updated some style dependencies cf #2279 , so you'll need to update the diffusers with

pip install --upgrade -e .["quality"]

Rebase the branch and then run make style.

Let me know if you need any help with this.

isamu-isozaki · 2023-02-08T15:54:07Z

Sounds good! Fixing merge conflict and style now

isamu-isozaki · 2023-02-08T16:13:20Z

@patil-suraj @williamberman Should be fixed! One small note. When testing I noticed that the default train_batch_size is 16 which might be a bit tough for non-high-end gpus.

isamu-isozaki · 2023-02-09T17:10:44Z

Interesting. I'll try fixing my version of black and fix the styles

isamu-isozaki · 2023-02-09T18:39:13Z

I do have black version 23.1.0 which should be exactly the same. I'll see if I can figure out the issue.

williamberman · 2023-02-13T06:52:47Z

examples/textual_inversion/textual_inversion.py

+            warnings.warn(
+                f"Future Warning: You are doing logging with validation_epochs={args.validation_epochs}."
+                " validation_epochs is being deprecated in favor of validation_steps.",
+                FutureWarning,
+                stacklevel=2,


Could we move the warning to directly after parse_args

Also see: https://github.com/huggingface/diffusers/pull/2178/files#r1104474041

examples/textual_inversion/textual_inversion.py

williamberman · 2023-02-13T06:59:56Z

Looks basically there :)

examples/textual_inversion/textual_inversion.py

patrickvonplaten · 2023-02-13T13:30:04Z

examples/textual_inversion/textual_inversion.py

        help=(
-            "Run validation every X epochs. Validation consists of running the prompt"
+            "Deprecated in favor of validation_steps. Run validation every X epochs. Validation consists of running the prompt"


Can we add the deprecation statement right below the argparsing like @williamberman mentioned below and do something like:

if args.validation_epochs is not None: warnings.warn(f"Deprecate ..... Please make sure to use `validation_steps` instead in the future. Setting `args.validation_steps` to {args.validation_epochs * num_samples_per_epoch}.") args.validation_steps =

once this is done args.validation_epochs doesn't have to be used anymore and it'll also be much easier to remove in the future.

@patrickvonplaten Great point! For the length of the validation steps, I put it after the dataset creation since that is where we count the number of images in the folder/find out the length of each epoch

isamu-isozaki · 2023-02-13T15:48:55Z

@patrickvonplaten @williamberman Thanks a bunch for the reviews! Will fix now

….com/isamu-isozaki/diffusers into textual_inv_make_save_log_both_steps

Co-authored-by: Will Berman <[email protected]>

isamu-isozaki · 2023-02-13T21:25:04Z

@patrickvonplaten @williamberman Should be fixed! For the deprecation warning, I did it after the dataset creation because that's when we know the size of each epoch. For everything else should be fixed!

isamu-isozaki · 2023-02-13T21:26:34Z

Testing now

isamu-isozaki · 2023-02-13T21:52:02Z

Ok, I don't know if this is a bug with the code but for the first image, it doesn't log. But after the second one, it starts logging. Will double-check.

isamu-isozaki · 2023-02-13T21:52:32Z

By not logging, I mean the pipeline gets run but it doesn't go to wandb.

isamu-isozaki · 2023-02-13T22:48:17Z

Actually, I noticed this in general with wandb. Sorry, prob not relevant.

isamu-isozaki · 2023-02-13T22:48:38Z

Anyways should be done!

patrickvonplaten · 2023-02-14T21:21:03Z

examples/textual_inversion/textual_inversion.py

+            FutureWarning,
+            stacklevel=2,
+        )
+        args.validation_steps = args.validation_epochs * len(train_dataset)


@patrickvonplaten thank you!

patrickvonplaten

Looks good to me!

patrickvonplaten · 2023-02-16T09:54:26Z

@williamberman could you check here?

patrickvonplaten · 2023-03-02T18:04:34Z

Thanks a lot @isamu-isozaki

isamu-isozaki · 2023-03-02T18:11:56Z

@patrickvonplaten np! Always happy to contribute

* Initial commit * removed images * Made logging the same as save * Removed logging function * Quality fixes * Quality fixes * Tested * Added support back for validation_epochs * Fixing styles * Did changes * Change to log_validation * Add extra space after wandb import * Add extra space after wandb Co-authored-by: Will Berman <[email protected]> * Fixed spacing --------- Co-authored-by: Will Berman <[email protected]>

isamu-isozaki added 5 commits January 31, 2023 11:08

Initial commit

82b445f

removed images

67bb426

Made logging the same as save

eba7a32

Removed logging function

fb42636

Quality fixes

561f646

patrickvonplaten requested review from pcuenca and patil-suraj February 3, 2023 16:49

patil-suraj reviewed Feb 7, 2023

View reviewed changes

Merged:

6bfae4f

isamu-isozaki added 2 commits February 8, 2023 10:56

Quality fixes

8f060e0

Tested

c03a7f6

isamu-isozaki added 2 commits February 9, 2023 11:28

Added support back for validation_epochs

78257c3

Fixing styles

6cd8ad9

Merge branch 'main' into textual_inv_make_save_log_both_steps

6448d4b

williamberman reviewed Feb 13, 2023

View reviewed changes

examples/textual_inversion/textual_inversion.py Outdated Show resolved Hide resolved

williamberman reviewed Feb 13, 2023

View reviewed changes

examples/textual_inversion/textual_inversion.py Show resolved Hide resolved

patrickvonplaten reviewed Feb 13, 2023

View reviewed changes

examples/textual_inversion/textual_inversion.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Feb 13, 2023

View reviewed changes

examples/textual_inversion/textual_inversion.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Feb 13, 2023

View reviewed changes

isamu-isozaki and others added 6 commits February 13, 2023 15:51

Did changes

3f7a20f

Merge branch 'textual_inv_make_save_log_both_steps' of https://github…

dd60d79

….com/isamu-isozaki/diffusers into textual_inv_make_save_log_both_steps

Change to log_validation

5a94681

Add extra space after wandb import

95345b5

Add extra space after wandb

fffffce

Co-authored-by: Will Berman <[email protected]>

Fixed spacing

8a9832b

patrickvonplaten reviewed Feb 14, 2023

View reviewed changes

patrickvonplaten approved these changes Feb 14, 2023

View reviewed changes

patrickvonplaten requested review from patil-suraj and williamberman February 14, 2023 21:21

This was referenced Feb 14, 2023

[Dreambooth] Add a function to log interim generation #1714

Closed

Save sample images while training Dreambooth & Textual Inversion #1993

Closed

patrickvonplaten merged commit d9b9533 into huggingface:main Mar 2, 2023

yiyixuxu mentioned this pull request Mar 5, 2023

add intermediate logging for dreambooth training script #2557

Merged

Textual inv make save log both steps #2178

Textual inv make save log both steps #2178

Uh oh!

Conversation

isamu-isozaki commented Jan 31, 2023

Uh oh!

HuggingFaceDocBuilderDev commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isamu-isozaki commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williamberman commented Feb 7, 2023

Uh oh!

isamu-isozaki commented Feb 7, 2023

Uh oh!

isamu-isozaki commented Feb 7, 2023

Uh oh!

patil-suraj commented Feb 8, 2023

Uh oh!

isamu-isozaki commented Feb 8, 2023

Uh oh!

isamu-isozaki commented Feb 8, 2023

Uh oh!

isamu-isozaki commented Feb 9, 2023

Uh oh!

isamu-isozaki commented Feb 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

williamberman commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isamu-isozaki commented Feb 13, 2023

Uh oh!

isamu-isozaki commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isamu-isozaki commented Feb 13, 2023

Uh oh!

isamu-isozaki commented Feb 13, 2023

Uh oh!

isamu-isozaki commented Feb 13, 2023

Uh oh!

isamu-isozaki commented Feb 13, 2023

Uh oh!

isamu-isozaki commented Feb 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten commented Feb 16, 2023

Uh oh!

HuggingFaceDocBuilderDev commented Jan 31, 2023 •

edited

Loading

isamu-isozaki commented Jan 31, 2023 •

edited

Loading

isamu-isozaki commented Feb 13, 2023 •

edited

Loading