Skip to content

Conversation

@bakkot
Copy link
Contributor

@bakkot bakkot commented Sep 3, 2022

5454a0e broke img2img on MPS and when using -v.

PR'd against the main branch because the main branch is currently broken, but I can switch to development if you'd prefer.

(I also added a second commit fixing an assertion.)

@bakkot bakkot requested a review from lstein September 3, 2022 20:38
@bakkot bakkot mentioned this pull request Sep 3, 2022
@mgcrea
Copy link
Contributor

mgcrea commented Sep 3, 2022

Did not dig into the code but tested the patch and it looks like it does cumulate variations, and I don't think that is the behaviour we want here.

We want n-generated variations against the base image and not against the last generated variation.

Also not sure what this patch has that is specific of MPS.

@bakkot
Copy link
Contributor Author

bakkot commented Sep 3, 2022

Did not dig into the code but tested the patch and it looks like it does cumulate variations, and I don't think that is the behaviour we want here.

Huh, that's surprising. It really shouldn't, from the code, and that's not what it looks like in my own testing. Are you sure it didn't just end up that way by chance? Can you run it again a couple times?

If it's consistent, can you provide an exact image and prompt (including seed) so that I can see if I can reproduce it?

Also not sure what this patch has that is specific of MPS.

The relevant lines are here - we do our own noise initialization on MPS, rather than allowing the diffusers to do it, so that we can do it on the CPU, which is more deterministic for MPS.

That is, _get_noise was broken entirely for img2img, and that function is called in two scenarios: when doing variations, or when on MPS.

@mgcrea
Copy link
Contributor

mgcrea commented Sep 3, 2022

Might have been by chance indeed, will run some more tests and share a seed if I can reliably reproduce it (was using the prompt from the readme). In my case the current main is not obviously broken when using variations (not sure if expected). Anyway, sorry for the noise and thanks for the detailed explanations!

@bakkot
Copy link
Contributor Author

bakkot commented Sep 3, 2022

my case the current main is not obviously broken when using variations (not sure if expected)

Sorry, it's specifically when using variations combined with img2img, i.e. when you pass an init_img, that it's broken.

@jkhales
Copy link

jkhales commented Sep 4, 2022

this fixed img2img on my MPS as well 🙏

@magnusviri
Copy link
Contributor

I should've noticed this is the same bug as I posted a few hours ago.. It's because a function was moved and the scope was changed. I am not sure this pull request is the best way to fix it. We could probably just move the function back. lstein is looking into it.

Copy link
Contributor

@magnusviri magnusviri left a comment

Choose a reason for hiding this comment

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

This looks good.

@magnusviri magnusviri merged commit 751283a into invoke-ai:main Sep 4, 2022
@bakkot bakkot deleted the fix-img2img-variations branch September 4, 2022 16:50
@tildebyte
Copy link
Contributor

@magnusviri; Please never merge PRs directly to 'main'. All PR's (except maybe in the case of documentation corrections) MUST merge into the 'development' branch; merges to 'main' MUST only happen when @lstein cuts a release.

We haven't hashed out how hotfixes work (i.e. really bad breaking bugs or security issues), but generally speaking, you'd open a PR, mark it prominently as a HOTFIX, it would get tested, reviewed, merged to 'dev', and then lstein would immediately merge to main and cut a hotfix release.

The practical issue here (aside from the issue of having fixes in 'main' which aren't in a release and haven't necessarily been tested thoroughly) is that 'main' and 'development' will diverge, and this can easily result in someone having to spend a lot of time manually rebasing the two before 'dev' can be properly merged to 'main' for the next release. In this specific case, the risk is low as it only involves small changes in a single file, but it's very bad practice.

@magnusviri
Copy link
Contributor

@tildebyte yeah sorry about that. After I did it I realized my mistake. I was going to say sorry somewhere but I have a hard time keeping track of where people are talking about things and it was late so I went to bed and forgot the next day. It won't happen again.

@tildebyte
Copy link
Contributor

No worries!

austinbrown34 pushed a commit to cognidesign/InvokeAI that referenced this pull request Dec 30, 2022
* fix img2img variations

* fix assert for variation_amount
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.

5 participants