-
Notifications
You must be signed in to change notification settings - Fork 448
Add prompt ID to interrupt API call #4393
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
Conversation
| prompt, | ||
| remove: { name: 'Cancel', cb: () => api.interrupt() } | ||
| // prompt[1] is the prompt id | ||
| remove: { name: 'Cancel', cb: () => api.interrupt(prompt[1]) } |
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.
prompt[1] is not the most readable, but this bit of code is only used by the legacy UI and it's somewhat complicated to have nicer types here, so I did a low-effort thing.
|
Hi @deepanjanroy, will this change be later supported by a change to the Currently, the
On the frontend, we create abstracted ComfyUI_frontend/src/scripts/api.ts Lines 755 to 762 in f5b03f3
Is there a known limitation for these approaches? Or is this something specific for cloud? |
|
Hi @christian-byrne! We needed it for cloud yes, don't think we need to support it right now on regular Comfy backend. The regular comfy backend only supports running one task at a time so it doesn't need to know the prompt id. (cc @robinjhuang if I got anything wrong) Re: why not use cancel queue / delete history: I think none of those cancel a currently executing task. That's exclusively handled by interrupt. |
|
Yeah echoing what Deep said. Interrupt is meant to cancel currently running jobs, so the intention is different from cancel (cancel items in the queue). Is this a harmless change on FE (even if ComfyUI backend doesn't use it for now)? Or are there potential side effects? @christian-byrne Edit: FYI this is not urgent at all so it can wait. |
|
Looks like there's a PR to implement this in core too. https://github.com/comfyanonymous/ComfyUI/pull/8308/files |
fd82ef5 to
c51db04
Compare
|
Okay makes sense, thanks for explanation. I don't forsee side effects. This could be merged currently if necessary. |
a558966 to
b4a2459
Compare
|
@christian-byrne / any other code owners: would you mind approving the PR and merging it if there are no other blockers? |
The Comfy UI backend currently ignores the post body.
┆Issue is synchronized with this Notion page by Unito