-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
allow interrupt generation in command line (Ctrl+C) #15210
allow interrupt generation in command line (Ctrl+C) #15210
Conversation
print(f"Unknown server command: {server_command}") | ||
break | ||
except KeyboardInterrupt: | ||
if shared.cmd_opts.allow_interrupt_generation_in_command_line and shared.state.job != "" and not shared.state.interrupted: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared.state.job != ""
- for case if there's not generation in process. If is set to "" in gpu job queue wrapper after jobnot shared.state.interrupted
- for double pressed Ctrl+C
@@ -125,3 +125,4 @@ | |||
parser.add_argument("--unix-filenames-sanitization", action='store_true', help="allow any symbols except '/' in filenames. May conflict with your browser and file system") | |||
parser.add_argument("--filenames-max-length", type=int, default=128, help='maximal length of filenames of saved images. If you override it, it can conflict with your file system') | |||
parser.add_argument("--no-prompt-history", action='store_true', help="disable read prompt from last generation feature; settings this argument will not create '--data_path/params.txt' file") | |||
parser.add_argument("--allow-interrupt-generation-in-command-line", action='store_true', help="set interrupt generation flag on KeyboardInterrupt signal (Ctrl+C)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe need to do shorter flag
@@ -161,7 +161,7 @@ def sigint_handler(sig, frame): | |||
|
|||
os._exit(0) | |||
|
|||
if not os.environ.get("COVERAGE_RUN"): | |||
if not (os.environ.get("COVERAGE_RUN") or shared.cmd_opts.allow_interrupt_generation_in_command_line): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to move sigint_handler
entry inside webui.sh under except KeyboardInterrupt
?
else:
if os.environ.get("COVERAGE_RUN":
...
os._exit(0)
print('Caught KeyboardInterrupt, stopping...')
server_command = "stop"
break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If yes, it's better to do in a new PR
Is there a need for this? The UI button works. |
I also don't see much of the use case for this webui is not a CLI app, we have no CLI interface |
I really often press Ctrl+C in console to interrupt generation even if it requires server restart. CLI progress bar is much more informative. 🤷🏻♀️ Also the browser with this gradio session can be closed |
Vice versa, I think people who didn't enable it, and used to use command line, might accidentally kill the server, thinking they were interrupting the generation.
It's hard to misunderstand because State prints into console if job is ended
|
no, you set must have set you loglevel above INFO, default should be WARN in fact I believe there's no other clear indication that a job has ended hence a slight misunderstanding but that's besides the point my main argument is that web UI is not a CLI app the only time that I can see people accidentally killing the server with the keyboard interrupt is when they're actually trying to use |
I have the default. I traced it after:
And it's None (argparser's default)
But I didn't make new feature by default for everyone :( Only if |
too be honest I don't really care if this gets merged or not as this is an opt-in feature
|
the default loglevel for python is wanring |
I've found. Deoldify extension overrides it. Will make PR to them Also to consider your notice I can add enabling loglevel.INFO for shared_state logger if this flag provided |
Also there is an other use-case: if my laptop discharged or android killed browser how it likes to do while a big generation, it's possible to interrupt without shutting down the server |
@@ -7,6 +7,8 @@ | |||
from typing import Optional | |||
|
|||
log = logging.getLogger(__name__) | |||
if shared.cmd_opts.allow_interrupt_generation_in_command_line: | |||
log.setLevel(logging.INFO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't do this
this is bad
ie I set it log level to debug and hear you force it to INFO
this is not the way to solve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check are cmd opt and env var None, and do it only in this case. Would it be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about this 2 line of code, I am just saying that you should change set the loglevel for the user
that like adding a SpenserCai/sd-webui-deoldify#60 issue but in base webui
as for this PR
I made my point already
I don't think adding a CLI gen interrupt is constituent with with webui as it is a none CLI application
I still think the use case is very niche
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that like adding a SpenserCai/sd-webui-deoldify#60 issue but in base webui
In this issue extension sets global logger level, as I see. In my case log = logger.getLogger(__name__)
, so it's changed only for this file
upd yes, I had HTTP logs (And didn't know how to remove them) before deoldify fix, and it disappeared. After this shared_state logger came back only job logs. So it's not globally. I will push commit to not override if user set cmd flag of env variable
..... the reconnect function seem to be broken |
If I understand you right, you are talking about the vortex emoji button next to styles. I saw it few times, but it works only for completed generations as I understand. And it's unstable, I never relied on it. Or did you mean my patch broke something? |
the restore porgress button is somehow "slightly" broken between 1.7 and 1.8 (nothing to do with this PR)
really, seem to be very stable for me I added in the fix PR so that if when you use it reconnect it also show the |
My decision is, if you want this, make a script/extension. If something is needed from webui (callbacks) for this, we can add them. |
Okay! I will make callbacks for interruption signal, and turn it into an extension |
Description
Ctrl+C in console will set interrupt flag, if
--allow-interrupt-generation-in-command-line
cmd option is usedChecklist: