sd-webui-deforum icon indicating copy to clipboard operation
sd-webui-deforum copied to clipboard

Update to split sampler and scheduler selections

Open rhyswynn opened this issue 1 year ago • 17 comments

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.

rhyswynn avatar Apr 22 '24 11:04 rhyswynn

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

andyxr avatar Apr 24 '24 10:04 andyxr

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 avatar Apr 24 '24 11:04 rhyswynn

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

rhyswynn avatar Apr 24 '24 11:04 rhyswynn

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

andyxr avatar Apr 25 '24 12:04 andyxr

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.

rhyswynn avatar Apr 25 '24 14:04 rhyswynn

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!)

andyxr avatar Apr 25 '24 17:04 andyxr

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 avatar Apr 25 '24 20:04 rhyswynn

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

rhyswynn avatar Apr 25 '24 23:04 rhyswynn

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

rhyswynn avatar Apr 26 '24 10:04 rhyswynn

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

Ditto!! :-)

andyxr avatar Apr 26 '24 11:04 andyxr

Can this be merged now?

andyxr avatar May 04 '24 08:05 andyxr

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

rhyswynn avatar May 04 '24 09:05 rhyswynn

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)

andyxr avatar May 04 '24 12:05 andyxr

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.

rhyswynn avatar May 04 '24 13:05 rhyswynn

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 avatar May 04 '24 20:05 andyxr

Shall we merge this puppy?

andyxr avatar May 13 '24 08:05 andyxr

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.

rhyswynn avatar May 13 '24 11:05 rhyswynn

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 avatar May 15 '24 00:05 rewbs