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

Fix 1137: Display ComfyUI Queue State #1156

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FeepingCreature
Copy link
Contributor

@FeepingCreature FeepingCreature commented Sep 6, 2024

Fix #1137: Query ComfyUI queue state and use it to display count of jobs in the queue before ours.

Also avoid clearing jobs that aren't ours.

@FeepingCreature FeepingCreature force-pushed the fix/1137-display-blocking-jobs branch 4 times, most recently from e28b1a1 to 04833db Compare September 6, 2024 19:55
@FeepingCreature FeepingCreature changed the title Fix issue 1137: Display ComfyUI Queue State Fix 1137: Display ComfyUI Queue State Sep 6, 2024
@FeepingCreature FeepingCreature force-pushed the fix/1137-display-blocking-jobs branch from 04833db to 29a40b2 Compare September 6, 2024 20:03
@FeepingCreature
Copy link
Contributor Author

Re why didn't I fix cancel current... so far as I can tell, this can only cancel another user's task if you do it right as a task starts and ends, and that's p much unavoidable due to comfy's anemic api, ie. interrupt not having a way to specify the job id we expect.

@FeepingCreature FeepingCreature force-pushed the fix/1137-display-blocking-jobs branch from 29a40b2 to 79836ec Compare September 6, 2024 20:10
@FeepingCreature FeepingCreature force-pushed the fix/1137-display-blocking-jobs branch from 79836ec to 252ddd3 Compare September 6, 2024 21:23
ai_diffusion/client.py Outdated Show resolved Hide resolved
ai_diffusion/client.py Outdated Show resolved Hide resolved
ai_diffusion/comfy_client.py Show resolved Hide resolved
except ValueError:
# probably just haven't gotten the notification yet
return
await self._report(ClientEvent.foreign_jobs, "", foreign_jobs=offset)
Copy link
Owner

Choose a reason for hiding this comment

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

Does it make sense to report the ID here? The UI would consider the job to be executing while waiting for its turn in the queue, which I think is how it already worked for the cloud client

Copy link
Contributor Author

@FeepingCreature FeepingCreature Sep 9, 2024

Choose a reason for hiding this comment

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

Well right, that's sort of my problem. Isn't that misleading? Cancelling the job doesn't fall under "cancel running" but "delete", because it isn't the active job from the server perspective.

The first job isn't any more in progress here than the second.

Copy link
Owner

Choose a reason for hiding this comment

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

Yea the cancel is a bit ambiguous in this case. But from user perspective I think it makes sense to track the active job (the earliest submitted by the user and not finished yet), and consider all follow-up jobs enqueued. The active job may be in a "waiting" state while the server is busy.

"Cancel active" should probably remove the active job from the server queue if it isn't processing yet in that case.

Copy link
Contributor Author

@FeepingCreature FeepingCreature Sep 10, 2024

Choose a reason for hiding this comment

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

I'm gonna leave that out for now cause I think you underestimate how ugly it's going to be. Right now the job queue tracks the server-side job queue pretty closely, which is why we can just call interrupt() to clear out the running job. But that's state that's distributed over like three classes, ie. client, jobs and the comfy server. If we start pretending that a queued job is an active one, even in the job queue, then the clients have to keep their own account of what's going on and redirect interrupt() to sometimes do /interrupt and sometimes /api/queue: delete. And clear_queue likewise has to look if the job has actually started yet, and if not, clear all but one queue element...

Anyway, ID added, queue depth is now tracked in model.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes I thought the interrupt call might just become:

if self._active_job:
  self._post("/interrupt")
else:
  self._post("/api/queue", {"delete": self._jobs[0].remote_id})

(something like that)

But maybe not?

Copy link
Owner

Choose a reason for hiding this comment

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

You mean any(queued) and any(executing) are equivalent? Mostly I guess. If you start a job and it errors early (connection failure or server enqueue failing directly) it never goes into executing.

Copy link
Contributor Author

@FeepingCreature FeepingCreature Sep 12, 2024

Choose a reason for hiding this comment

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

I mean, does it matter at that point? With that refactor, what is actually still dependent on executing?

edit: I mean, should I just make that change and we'll see?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm sure making executing implicit (oldest non-finished-non-cancelled job) works too, but I kinda like that it relies on client feedback and is an explicit state. I don't see a strong argument one way or the other, but if there's no strong reason to change, keep what already works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just hopelessly confused now. If you like that job state relies on client feedback, why do you want me to mark the first job, that the client has not started executing yet, as executing?

Copy link
Owner

Choose a reason for hiding this comment

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

Because I consider standing in line as part of executing my job! You know, pushing a little, trying to advance a spot or two when those in front are looking the other way. Things a good client does.

Call it "active" or "in_progress" or something if executing is so confusing. The important part is that the job is the focus of attention and something is being done about it. That's the job I care about, and which the UI progress bars and indicators refer to. Surely that's worth a special state? (compared to all the other jobs that are either old, finished and forgotten or distant future)

@FeepingCreature FeepingCreature force-pushed the fix/1137-display-blocking-jobs branch from 252ddd3 to 57d9ba3 Compare September 10, 2024 16:28
if message.queue_length is not None:
self.queue_length = message.queue_length
self.queue_length_changed.emit(message.queue_length)
if message.queue_length is None or message.queue_length == 0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the "don't start the job until it's actually running" thing moved to.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest to consider it started in all cases, like before (just add the queue length if available).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I don't see what it buys you. Why should we consider job started if the job is not started?

@FeepingCreature
Copy link
Contributor Author

Something's wrong with the code as it stands, queueing pictures makes the progress bar flicker. I'll check the logs tomorrow.

@FeepingCreature
Copy link
Contributor Author

Ah ftr I'm not gonna touch this until end of the week cause I'm on a conference :)

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.

Display place in ComfyUI queue for processing
2 participants