Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to split sampler and scheduler selections #968

Merged
merged 4 commits into from
May 15, 2024

Conversation

rhyswynn
Copy link
Contributor

This will add an additional scheduler drop down selector, and pass the additional scheduler parameter to the SD pipeline, based on recent changes to the Automatic1111 approach.

@andyxr
Copy link
Contributor

andyxr commented Apr 24, 2024

What happens when users try to load settings files from an earlier version of Deforum? Is this case handled?

@rhyswynn
Copy link
Contributor Author

It worked for me, I loaded one of my old settings files and there was no issue. The logic in settings.py looks to see if the key is there before setting it, and defaults.py is updated to include a default value of Automatic, just like A1111 does.

@rhyswynn
Copy link
Contributor Author

I'm happy to push these changes to a branch instead, or you could test it from my fork.

@andyxr
Copy link
Contributor

andyxr commented Apr 25, 2024

It worked for me, I loaded one of my old settings files and there was no issue. The logic in settings.py looks to see if the key is there before setting it, and defaults.py is updated to include a default value of Automatic, just like A1111 does.

OK. So you can load an old settings file, and the sampler from that file gets successfully turned into the correct sampler/scheduler combo? Just double checking..

@rhyswynn
Copy link
Contributor Author

Good point, I think it would result in the default values, if the keys don't match it won't have a value to update. I'll review the logic later today. I am confident it is a non-breaking concern, but it would be a more complete solution that way.

@andyxr
Copy link
Contributor

andyxr commented Apr 25, 2024

Good point, I think it would result in the default values, if the keys don't match it won't have a value to update. I'll review the logic later today. I am confident it is a non-breaking concern, but it would be a more complete solution that way.

Yeah it shouldn't be too tricky to map the old sampler string to the new pair. If it turns out to be too tricky, we can always put out a warning: This version will set your sampler/scheduler to the default Euler A (or whatever it is!)

@rhyswynn
Copy link
Contributor Author

I updated it. defaults.py I added an additional LCM sampler that's showing in A1111 txt to image. I have no idea how that logic in settings.py was working before, because it was checking if a string value was an int, so the logic to set the sampler values from the settings file couldn't have been working. I added logic to parse the value and set the scheduler value if the legacy value was a combination. I tested it out and it successfully loaded the old settings with the correct sampler/scheduler combination.

@rhyswynn
Copy link
Contributor Author

this needs more work, don't approve it yet please

@rhyswynn
Copy link
Contributor Author

Ok, all good now, thank you for your patience! Python isn't my native language :)

@andyxr
Copy link
Contributor

andyxr commented Apr 26, 2024

Ok, all good now, thank you for your patience! Python isn't my native language :)

Ditto!! :-)

@andyxr
Copy link
Contributor

andyxr commented May 4, 2024

Can this be merged now?

@rhyswynn
Copy link
Contributor Author

rhyswynn commented May 4, 2024

Yep, it's working fine for me with the updates I made. Thank you!

@andyxr
Copy link
Contributor

andyxr commented May 4, 2024

Yep, it's working fine for me with the updates I made. Thank you!

So, shall we pull the trigger and merge? Also..... (!) does it work on A1111 v1.8 or is it a V1.9 thing only? I think we can live with it being a V1.9 only thing; we did a similar fix when 1.8 came out (i.e Deforum required V1.8)

@rhyswynn
Copy link
Contributor Author

rhyswynn commented May 4, 2024

I don't think it would work, because the sd pipeline in the older version is only expecting the sampler parameter, not the sampler and the scheduler. That's the never ending challenge with these kind of add ons to another package. How much time and effort do we spend to accommodate the edge case of someone who's had A1111 for a while and suddenly wants to use Deforum? It wouldn't be technically challenging to add a check, but I don't have the time or the motivation for doing that and doing a downgrade to test it. I vote to add a note to the readme. Let me know your thoughts and I'll update the pull request.

@andyxr
Copy link
Contributor

andyxr commented May 4, 2024

I don't think it would work, because the sd pipeline in the older version is only expecting the sampler parameter, not the sampler and the scheduler. That's the never ending challenge with these kind of add ons to another package. How much time and effort do we spend to accommodate the edge case of someone who's had A1111 for a while and suddenly wants to use Deforum? It wouldn't be technically challenging to add a check, but I don't have the time or the motivation for doing that and doing a downgrade to test it. I vote to add a note to the readme. Let me know your thoughts and I'll update the pull request.

Yeah, I get it. Like I said, we had to force everyone to use A1111 v1.8, so why not 1.9. Let's merge it and release.

@andyxr andyxr requested a review from rewbs May 5, 2024 08:09
@andyxr
Copy link
Contributor

andyxr commented May 13, 2024

Shall we merge this puppy?

@rhyswynn
Copy link
Contributor Author

Shall we merge this puppy?

Yes please. Do I need to take some action? I'm sorry, I have not contributed to someone else's repo before.

@rewbs
Copy link
Contributor

rewbs commented May 15, 2024

Do I need to take some action? I'm sorry, I have not contributed to someone else's repo before.

Nothing more needed from you @rhyswynn – I am reviewing now and it looks good. I will merge shortly.

I think we can live with it being a V1.9 only thing

Agreed. I have created a release with the current state of the repo (before merging this) tagged as the version to use for a1111 v1.8: https://github.com/deforum-art/sd-webui-deforum/releases/tag/a1111-v1.8

@rewbs rewbs merged commit 91df0fa into deforum-art:automatic1111-webui May 15, 2024
Olehidubo pushed a commit to Olehidubo/sd-webui-deforum-works-without-cn that referenced this pull request Jul 7, 2024
…1-webui"

This reverts commit 91df0fa, reversing
changes made to 3224268.
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.

None yet

3 participants