-
-
Notifications
You must be signed in to change notification settings - Fork 387
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
Update to split sampler and scheduler selections #968
Conversation
What happens when users try to load settings files from an earlier version of Deforum? Is this case handled? |
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. |
I'm happy to push these changes to a branch instead, or you could test it from my fork. |
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.. |
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!) |
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. |
this needs more work, don't approve it yet please |
Ok, all good now, thank you for your patience! Python isn't my native language :) |
Ditto!! :-) |
Can this be merged now? |
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) |
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. |
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. |
Nothing more needed from you @rhyswynn – I am reviewing now and it looks good. I will merge shortly.
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 |
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.