Skip to content

allow interrupt generation in command line (Ctrl+C)#15210

Closed
light-and-ray wants to merge 3 commits into
AUTOMATIC1111:devfrom
light-and-ray:allow_interrupt_generation_in_command_line
Closed

allow interrupt generation in command line (Ctrl+C)#15210
light-and-ray wants to merge 3 commits into
AUTOMATIC1111:devfrom
light-and-ray:allow_interrupt_generation_in_command_line

Conversation

@light-and-ray

@light-and-ray light-and-ray commented Mar 11, 2024

Copy link
Copy Markdown
Contributor

Description

Ctrl+C in console will set interrupt flag, if --allow-interrupt-generation-in-command-line cmd option is used

Checklist:

Comment thread webui.py
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:

@light-and-ray light-and-ray Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

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 job
  • not shared.state.interrupted - for double pressed Ctrl+C

Comment thread modules/cmd_args.py
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)")

Copy link
Copy Markdown
Contributor Author

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

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

@AUTOMATIC1111

Copy link
Copy Markdown
Owner

Is there a need for this? The UI button works.

@w-e-w

w-e-w commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

I also don't see much of the use case for this
also I think for those whe enable this, they people might accidentally kill the server if they sent a keyboard interrupt thinking that a job is still running, as the behavior changes based on what the server is doing at the moment

webui is not a CLI app, we have no CLI interface
so one is either using the webui or api
if using the UI use the button or hotkey to interupt
if via api then theres is the interrupt api route
adding a interupt in CLI for a non CLI app, the case would be extremely niche

@light-and-ray

light-and-ray commented Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

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

@light-and-ray

light-and-ray commented Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

those whe enable this, they people might accidentally kill the server if they sent a keyboard interrupt thinking that a job is still running

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.

thinking that a job is still running

It's hard to misunderstand because State prints into console if job is ended

Starting job task(7dob9zajuf7xlss)
...
Ending job task(7dob9zajuf7xlss) (3.14 seconds)

@w-e-w

w-e-w commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

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
which dose not print when a job ends

in fact I believe there's no other clear indication that a job has ended
the closest thing is the progress bar (enabled by default)
but it is not guaranteed because there's still other things going on after progress bar has rached 100%

hence a slight misunderstanding
normally there is no indication of when a job is really finished the closest thing is the progress bar but that is still not a guarantee indication
and so after the progress bar without looking a the web client you can't know what stat a server is in, worse even if the progress bar is disabled


but that's besides the point my main argument is that web UI is not a CLI app
for a none CLI app if it has a terminal, the termal is normally only for info and for kiling the process by sending a keyboard interupt or when the termianl is closed

the only time that I can see people accidentally killing the server with the keyboard interrupt is when they're actually trying to use ctrl + c to copy text

@light-and-ray

Copy link
Copy Markdown
Contributor Author

no, you set must have set you loglevel above INFO, default should be WARN

I have the default. I traced it after:

def setup_logging(loglevel):
    if loglevel is None:
        loglevel = os.environ.get("SD_WEBUI_LOG_LEVEL")

And it's None (argparser's default)

in fact I believe there's no other clear indication that a job has ended

But I didn't make new feature by default for everyone :( Only if --allow-interrupt-generation-in-command-line

@w-e-w

w-e-w commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

too be honest I don't really care if this gets merged or not as this is an opt-in feature
I just feel the use case is very niche

too slow to edit this into the end of last message before you replied

@w-e-w

w-e-w commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

the default loglevel for python is wanring
https://docs.python.org/3/howto/logging.html
so irrc if it is set to none it should be warn by default
so likey you have some configuration some overrides it

@light-and-ray

Copy link
Copy Markdown
Contributor Author

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

@light-and-ray

Copy link
Copy Markdown
Contributor Author

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

Comment thread modules/shared_state.py

log = logging.getLogger(__name__)
if shared.cmd_opts.allow_interrupt_generation_in_command_line:
log.setLevel(logging.INFO)

Copy link
Copy Markdown
Collaborator

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

@light-and-ray light-and-ray Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Collaborator

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

@light-and-ray light-and-ray Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

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

@w-e-w

w-e-w commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

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

..... the reconnect function seem to be broken

@light-and-ray

Copy link
Copy Markdown
Contributor Author

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?

@w-e-w w-e-w mentioned this pull request Mar 11, 2024
4 tasks
@w-e-w

w-e-w commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

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)
I just made a fix just now

unstable

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 interrupt | skip if the server is still working on the job

@AUTOMATIC1111

Copy link
Copy Markdown
Owner

My decision is, if you want this, make a script/extension. If something is needed from webui (callbacks) for this, we can add them.

@light-and-ray

Copy link
Copy Markdown
Contributor Author

Okay! I will make callbacks for interruption signal, and turn it into an extension

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.

3 participants