diffusers icon indicating copy to clipboard operation
diffusers copied to clipboard

Textual inv make save log both steps

Open isamu-isozaki opened this issue 1 year ago • 19 comments

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.

isamu-isozaki avatar Jan 31 '23 17:01 isamu-isozaki

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

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

isamu-isozaki avatar Jan 31 '23 19:01 isamu-isozaki

@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.

williamberman avatar Feb 07 '23 20:02 williamberman

@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 avatar Feb 07 '23 21:02 isamu-isozaki

Will fix merge conflict

isamu-isozaki avatar Feb 07 '23 21:02 isamu-isozaki

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.

patil-suraj avatar Feb 08 '23 08:02 patil-suraj

Sounds good! Fixing merge conflict and style now

isamu-isozaki avatar Feb 08 '23 15:02 isamu-isozaki

@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 avatar Feb 08 '23 16:02 isamu-isozaki

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

isamu-isozaki avatar Feb 09 '23 17:02 isamu-isozaki

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

isamu-isozaki avatar Feb 09 '23 18:02 isamu-isozaki

Looks basically there :)

williamberman avatar Feb 13 '23 06:02 williamberman

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

isamu-isozaki avatar Feb 13 '23 15:02 isamu-isozaki

@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 avatar Feb 13 '23 21:02 isamu-isozaki

Testing now

isamu-isozaki avatar Feb 13 '23 21:02 isamu-isozaki

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 avatar Feb 13 '23 21:02 isamu-isozaki

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

isamu-isozaki avatar Feb 13 '23 21:02 isamu-isozaki

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

isamu-isozaki avatar Feb 13 '23 22:02 isamu-isozaki

Anyways should be done!

isamu-isozaki avatar Feb 13 '23 22:02 isamu-isozaki

@williamberman could you check here?

patrickvonplaten avatar Feb 16 '23 09:02 patrickvonplaten

Thanks a lot @isamu-isozaki

patrickvonplaten avatar Mar 02 '23 18:03 patrickvonplaten

@patrickvonplaten np! Always happy to contribute

isamu-isozaki avatar Mar 02 '23 18:03 isamu-isozaki