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

Added New Scheduler: Simple KES - Karras Exponential Scheduler #16598

Closed
wants to merge 6 commits into from

Conversation

Kittensx
Copy link

Description

Added support for a new custom scheduler which combines Karras with Exponential with customizable options that configures the scheduler with the use of yaml config file.
Includes a bat file in root which can update the venv for required dependancies
Includes watchdog which will monitor the yaml config file and update the scheduler with new config options. Watchdog is only looking for config changes in the yaml file.

Screenshots/videos:

Checklist:

forgot to include this in my lasst commit
Copy link
Collaborator

@w-e-w w-e-w left a comment

Choose a reason for hiding this comment

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

I think I see what you trying to do
but what is in this PR is just add 3 unused files to the repo, in other words it serves no function
it's incomplete at best
if a PR is not read for review the you shoud make it a draft and not a open PR

if you are actually wish to make a PR into the repo then probably then I think it shoud at least fulfill these requirements

  • it needs to be integrated webui
    • you should be able to set those parameters through the web UI / api and not thourgh
      • it should not be only accessible via editing the file manually through local means
    • generation parameters should be saved to the image
    • generation parameters should be able to be parsed back from pnginfo / infotext
  • files that are meant to be edited by the user should not be stored in modules
    • if a template like file for user to change then that's file can never be changed by us after its initial creation
      config
  • correct method of installing dependency
    • some libs such k-diffusion we don't use pip re clone the repo

this a incomplete list of the basic issues
it is not a review of or not this is useful or not


to be honest I feel like you should probably make this into an extension

@w-e-w w-e-w marked this pull request as draft October 27, 2024 21:49
@Kittensx
Copy link
Author

You can add it to code or not. It works in stable diffusion A1111 with the modified changes I posted to my github account.
I am putting together a release for Simple_KES.py shortly and would make importing the scheduler easier for projects like this.

@Kittensx Kittensx marked this pull request as ready for review October 28, 2024 05:07
@Kittensx
Copy link
Author

Also, I don't know enough about SD webui to make all the changes accessible through your code. I created a new scheduler. How you integrate it with your code is up to you.

@Kittensx
Copy link
Author

I added the install instructions on my page for how to integrate it with A1111 on their local machines. I am suggesting that you integrate with my scheduler, Simple_KES. I've udpated the SD_schedulers.py file so that the scheduler appears on the map. That's all you need to do to get functionality to use the scheduler. You don't need to modify the config file with my adjusted file here:
https://github.com/Kittensx/Simple_KES , modules/simple_karras_exponential_scheduler.py . That is the file I'm maintainng and updating.

@Kittensx
Copy link
Author

You're asking me to create an extension which adds functionality to use a new scheduler? You'd still need to add this file to the modules folder, and you'd still need to update the sd_schedulers.py file. If you want webui compatability, you'd need to look at the included config yaml file and use those variables in your code. Making it into an extension would not negate the need to add it to your modules folder and update sd_schedulers which I already did.

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 28, 2024

You'd still need to add this file to the modules folder, and you'd still need to update the sd_schedulers.py file. If you want webui compatability, you'd need to look at the included config yaml file and use those variables in your code

no you don't
you can add custom schedulers by updateing sd_schedulers.schedulers sd_schedulers.schedulers_map
put all your configs else where, llike a custom directory inside your extension store all the configs you like
code and and config should be separated

I wrote some a quick example https://github.com/w-e-w/Simple_KES
seems to work but unpolish
consider this a starting point on how to make this into an extension without the need of modifying webui
image

@w-e-w
Copy link
Collaborator

w-e-w commented Nov 2, 2024

as mentioned I suggest you making this into extension

the biggest advantage of having it as an extension is that
if you have features that you would like to add or bug fixes that you would like to make it can get added and push to use as rapidly
as opposed to having to wait for someone to review your pull request and for the changes to make its ways from dev to master

I have give you a starting point on how you can make a it as an extention without requiring modification modification to webui
so I suggest you read that example and convert your code into extension

see https://github.com/w-e-w/Simple_KES

however if you insist on wishing it to be in the main repo
then you actually have to provide "working code", as opposed to instructions on how to patch it into the current code base
as it stands my opinion is at this Pull Request is not actually a "Pull Request", but a "Feature request" post requesting adding of a new feature into the code base

by working code, I mean that some changes that when merged into the repo that will have an effect
not code that is just sitting on the side commonly known as dead code

moreover asking someone to patch WebUI or providing a script to do so is basically asking someone to break WebUI
as modification to the codebase will create uncommitted changes for git and preventing users who don't understand how to use git from pulling new updates in the future
as such a script or extension that modifies the codebase directly is considered "taboo"

in most casesit's actually possible to achieving the the result you need without actually haveing to modifying WebUIs codebase
but in the event that you do need some new callback hook to make something an extention work, you could submit a poll request for that

but as I have demonstrated in the my example, you actually don't need any changes to the existing webui code to make your scheduler work as extension


in my opinion you're trying to make a hybrid scheduler that is highly configurable
but I doubt the mass majority of users would be able to take advantage of this feature
as probably most uses don't have the understanding of what a scheduler even is

I really feels this is more suited as extension for advanced users that knows what they're doing and really wants to tinker with scheduler, as opposed to something that is should be the main repo

you can add your an extention to the index by makeing a PR to https://github.com/AUTOMATIC1111/stable-diffusion-webui-extensions

as such I'm closing this PR

@w-e-w w-e-w closed this Nov 2, 2024
@Kittensx
Copy link
Author

Kittensx commented Nov 2, 2024

"I doubt the mass majority of users would be able to take advantage of this feature" yea - since you called my code useless, you obviously have no understanding of schedulers.
"in my opinion you're trying to make a hybrid scheduler that is highly configurable" not trying to make. I made it. It works 100%.
" probably most uses don't have the understanding of what a scheduler even is" not my issue that people don't make efforts to learn what things are.
"I really feels this is more suited as extension for advanced users that knows what they're doing and really wants to tinker with scheduler," that's your opinion
", as opposed to something that is should be the main repo" and you think removing schedulers like 2M Karras ++ was the right call and adding new "automatic" options which frankly was not real improvement to the code based what that scheduler does, WAS an improvement, but adding a novel scheduler combining karras and exponential isn't an improvement? You added polyexponential which has no improved version over exponential. Look at any printout of both Exponential and Polyexponential and you'll see the same picture.

"you can add your an extention to the index by makeing a PR to https://github.com/AUTOMATIC1111/stable-diffusion-webui-extensions" and will you deny that application too because you think people won't use it?

@Kittensx
Copy link
Author

Kittensx commented Nov 2, 2024

"I have give you a starting point on how you can make a it as an extention without requiring modification modification to webui
so I suggest you read that example and convert your code into extension" I reviewed your code example and it doesn't work. You don't understand your own code. But I took your example and improved upon it, then rejected the design. Everytime you load the code it requires you to add it to schedulers. Not what I'm trying to accomplish here, but maybe thats just how extensions work. I wanted a permanent solution, and adding it the lines to the schedulers map is the permanent solution. Not adding some code to the main codebase doesn't allow it to update schedulers.

@w-e-w
Copy link
Collaborator

w-e-w commented Nov 3, 2024

to be honest at this point I'm not sure if I'm bad at writing or you just don't want to read
my words seems to be extremely twisted in your head, and does not represent what I'm saying
are you perhaps not that a native speaker of English and are using a translator to understand what I've written


"I doubt the mass majority of users would be able to take advantage of this feature" yea - since you called my code useless, you obviously have no understanding of schedulers.

I didn't call your code useless, I simply called that your code is not a PR
because it is "dead code", yes even if you provide a way to patch it in, it doesn't change that it is not that code
if you want to make a pull request you have to actually modify the existing code in other words you have to submit the patched version and not leave pre-patched patch resources
after a PR is merge it is supposed to be in a working state

let me play completely dumb then I don't understand anything about schedules I didn't worte the code myself, nor do 99.9% of the users
you need to explain what you did otherwise you basically did nothing

"in my opinion you're trying to make a hybrid scheduler that is highly configurable" not trying to make. I made it. It works 100%.

that is simply a way of speaking, you're reading too much into the simple phrase, but if you really want to read into it, then what you have made is a working prototype, that you are proposing your prototype as a final product
a PR needs to be a final product, your integration with the UI is not enough, users who are only using the web front end who doesn't have access to the file system is not able customize the schedulers

" probably most uses don't have the understanding of what a scheduler even is" not my issue that people don't make efforts to learn what things are.

if you made something that you wish to show others, but you're not able to represent it in a way that is understandable, that people simply don't understand you, and to them you made nothing

", as opposed to something that is should be the main repo" and you think removing schedulers like 2M Karras ++ was the right call and adding new "automatic" options which frankly was not real improvement to the code based what that scheduler does, WAS an improvement, but adding a novel scheduler combining karras and exponential isn't an improvement? You added polyexponential which has no improved version over exponential. Look at any printout of both Exponential and Polyexponential and you'll see the same picture.

just to make sure I'm pretty sure that there isn't a 2M Karras ++, only DPM++ 2M + Karras Schedule

I'm not sure what you're talking about DPM++ 2M Karras removed? it is there
image

automatic, is not and has never been a scheduler
it is simply a way of choosing a scheduler that AUTOMATIC1111 (the person) believes to be the best suited as a default option
it does not Implement any scheduler, it is just a UI design when we split combined Sampler dropdown two Sampler and Schedule, to increase ease of use
particulars for users that don't understand where to start with
if I wish to change is to specific scheduler it's always just a click away

I have never asserted that your code is useless, you are the one that is putting these words into my mouth
your code in this current state is not usable for the average user
it has nothing to do with the quality of the image produced
the quality of the image is not a part of my evaluation of this PR
my evaluation is totally based on if the code is usable or not
which it is objectively not
and so it is not a PR

my suggestion that it be made into an extension is a suggestion of practicality
as it offers several advantages mentioned above

"you can add your an extention to the index by makeing a PR to https://github.com/AUTOMATIC1111/stable-diffusion-webui-extensions" and will you deny that application too because you think people won't use it?

if my intention is to reject your extension because I believe it's not useful then I wouldn't even ask you to submit it


"I have give you a starting point on how you can make a it as an extention without requiring modification modification to webui
so I suggest you read that example and convert your code into extension" I reviewed your code example and it doesn't work. You don't understand your own code. But I took your example and improved upon it, then rejected the design. Everytime you load the code it requires you to add it to schedulers. Not what I'm trying to accomplish here, but maybe thats just how extensions work. I wanted a permanent solution, and adding it the lines to the schedulers map is the permanent solution. Not adding some code to the main codebase doesn't allow it to update schedulers.

I reviewed your code example and it doesn't work

it works, I produced two images with webui 1.10
if there is indeed a bug then you should point it out so that it can be fixed

the point of that example is simply to state that yes you can add schedulers to list of schedulers that is usable by a user from an extension

Everytime you load the code it requires you to add it to schedulers.

not entirely sure what you mean "by you",
if the "you" is the "user" no it's his automatically done by the code
if you is the developer or the python interpreter, then everything under the hood is added at runtime

I wanted a permanent solution, and adding it the lines to the schedulers map is the permanent solution

what do you mean by a "permanent solution", practically nothing is permanent in Python
there is no functional difference between adding a scheduler with web ui codebase versus adding it in an extension because it is running the same sequence of code
when the web UI code adds schedulers the python interpreter is doing a "dynamically", and in the example I wrote it is added in the same way
the only difference is at what time the code is loaded during startup sequence, and "syntactical" difference on how it is added
let us know functional difference under the hood and has no impact on the end result

if you don't want multiple config files, create a multiple schedule entries
just load one configuration statically what's the issue?
I made it scanner directory and add auu configs because I want 2 config so that I can quickly show that it is working using XYX grid

@w-e-w
Copy link
Collaborator

w-e-w commented Nov 3, 2024

I also gave you the option

however if you insist on wishing it to be in the main repo
then you actually have to provide "working code", as opposed to instructions on how to patch it into the current code base
as it stands my opinion is at this Pull Request is not actually a "Pull Request", but a "Feature request" post requesting adding of a new feature into the code base

as mentioned in the above reasons your code is not working code and so it is closed
if you wish to submit a PR then it has to be "working code" not "dead code"

just in case you misunderstand the term "dead code"
https://nordvpn.com/cybersecurity/glossary/dead-code/#:~:text=Dead%20code%20is%20programming%20that,on%20its%20processes%20or%20outputs

Dead code definition
Dead code is programming that will never be executed during a program's runtime and thus serves no purpose in the program's functionality. In some cases, dead code can also refer to redundant code that may be executed during a program’s runtime, but would have no impact on its processes or outputs.

the content in this Pull Request is code that is not reachable and will never be executed by webui
and as mentioned adding documentation or script to patch it in does not count

you are welcome to open a new PR with "live working code"

I've only suggest that you make it into an extension because I believe it is better
but you don't "have to"

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.

2 participants