mirror of
https://github.com/Comfy-Org/comfy-python-sdk.git
synced 2026-11-04 23:37:47 +00:00
[PR #137] [MERGED] feat(models): queue a model request with submit, subscribe and handle #137
Labels
No labels
audit:ai-readiness
audit:community
audit:github
audit:health-scoring
audit:policy
audit:release
audit:supply-chain
effort:medium
effort:small
effort:small
effort:trivial
priority:nice-to-have
priority:nice-to-have
priority:recommended
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Comfy-Org/comfy-python-sdk#137
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
📋 Pull Request Information
Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/137
Author: @mattmillerai
Created: 9/9/2026
Status: ✅ Merged
Merged: 9/14/2026
Merged by: @mattmillerai
Base:
main← Head:matt/be-12725-models-queue-handle📝 Commits (4)
4d03724feat(models): queue a model request with submit, subscribe and handlea7a8e11fix: address bot review comments on the queued model surface0f5426dfix: address bot review comments on the queue tests and result docsd83b342Merge origin/main into matt/be-12725-models-queue-handle📊 Changes
12 files changed (+2553 additions, -26 deletions)
View changed files
📝
CHANGELOG.md(+28 -0)📝
README.md(+114 -5)📝
src/comfy_low/transport.py(+243 -3)📝
src/comfy_sdk/__init__.py(+6 -0)📝
src/comfy_sdk/client.py(+3 -2)📝
src/comfy_sdk/exceptions.py(+11 -10)➕
src/comfy_sdk/model_requests.py(+717 -0)📝
src/comfy_sdk/models.py(+290 -2)📝
src/comfy_sdk/router_exceptions.py(+58 -0)📝
tests/conftest.py(+244 -2)➕
tests/test_models_queue.py(+830 -0)📝
tests/test_sync_async_parity.py(+9 -2)📄 Description
ELI-5
client.models.run(...)sits on the phone until your picture is ready. That's fine for a script and useless for a web server. This adds the other way to do the same thing:client.models.submit(...)hands the request to the queue and gives you back a ticket stub, and you go and get the picture later — from a different process if you like, because the stub is just two ids.subscribe(...)is the same thing with the waiting done for you and a callback for the progress bar. The queue itself is entirely the server's — this only knows how to ask "is it done yet?" politely, at the pace the server asks for.What this adds
models.submit(model, arguments)RequestHandlemodels.subscribe(model, arguments, on_queue_update=…, timeout=…)models.handle(model, request_id)RequestHandle.status()/.get()/.cancel()/.iter_events()Plus
QueueUpdate(the poll observation) androuter_exceptions.error_from_completion().The shape is deliberately the one
comfy_sdk.jobsalready uses for workflow jobs — submit, hold a handle, poll to terminal with adaptive backoff, collect or cancel — rather than a second idiom for the same thing. No queue behaviour is implemented client-side: ordering, admission, retries, timeouts, billing and expiry are all the server's, and this adds polling and ergonomics on top and nothing else.Three properties that are contract rather than implementation detail:
iter_eventsis the poll loop with its updates exposed, not SSE. It backs off adaptively and aRetry-Afterthe server names on a poll beats that schedule; aRetry-After: 0names no pace and falls back to the schedule rather than becoming a zero-delay loop.200is not a success here. The server reports a failed and a cancelled request asCOMPLETEDcarrying anerror_type, so every path that hands back a result runs the payload through the typedRouterErrormapping first. Anerror_typethis version has never heard of still raises the catchable base class.Idempotency-Keypersubmitcall. Two deliberate submits are two requests; a transport-level retry inside one call keeps the one key and replays the original. Every exception carries it on.idempotency_key, which is the only route back to a request whose response was lost.AsyncComfyawaits the same names with the same arguments in the same order. The existing introspection parity test discovers the newRequestHandle/AsyncRequestHandlepair and the three newmodelsmethods automatically; I extended its one explicit assertion to name them so the coverage cannot go vacuous.client.models.runis unchanged. The only edits tomodels.pyoutside the new methods are two import lines (git diffon that file shows exactly two removed lines, both imports).The one thing to look at first
The four routes are hand-bound and provisional:
The upstream contract that declares these four operations is authored but held, and the one-way sync into
spec/router-openapi.yamlstrips a held operation — so unlike_MODEL_RUN_PATH_TEMPLATE, whichtests/test_router_spec_contract.pypins against the vendored file byte for byte, these have nothing to pin them to. They are gathered in one place and nowhere else precisely so the sync that publishes them is a four-line diff plus the assertion that pins them, exactly as the run path was hand-bound before its own spec arrived.test_the_queue_routes_extend_the_run_routepins what can be pinned today (the relationship to the run path and the segments each template fills) and says in its docstring what to replace it with.Wire shapes assumed, all derived from what the surface itself forces rather than invented freely: the submit response names
request_id; the status body carriesstatus/queue_position/error_type;COMPLETEDis the one terminal status. That bothsubmitandhandleneed the model id and the request id is what says the request routes hang under the model-addressed prefix. Every one of these is parsed defensively — a missing field degrades rather than raising a decode error, an unknown status is treated as not yet terminal, and an unknownerror_typestill raises a catchableRouterError.Judgment calls
submit_async/subscribe_asyncaliases. The plan lists them as aliases; this repo'stests/test_sync_async_parity.pybans the_asyncsuffix anywhere on the public surface (_SUFFIXED),models.pystates there is deliberately norun_async"and there never will be", and the acceptance criterion itself asks for identical method names onAsyncComfy. Adding the aliases would mean weakening an existing guard to publish a second name for one operation, which cannot be withdrawn once released. I implemented the identical-names half and left the aliases out.AsyncModels.handleisasync defeven though it makes no request. The alternative was declaring it a deliberate asymmetry in the parity test's_SYNC_ON_BOTHtable. Awaiting it keeps the "addawait" contract exception-free, needs no edit to that table, and leaves room to validate server-side later without a breaking change.403 not_enabled, which arrives as the typedNotEnabled(tested). A second, client-side switch would be a local re-statement of a decision made somewhere authoritative — the same reason no queue behaviour is reimplemented here. The release-lifecycle half is the pre-release, below.error_typeonly counts inside the queue envelope. That body is the provider's own payload forwarded verbatim, and a partner model is free to have a field callederror_typein its native schema; raising on one would fail a generation that succeeded. So the result body is only read as a failure when it carries aCOMPLETEDstatus alongside it. The status read has no such ambiguity and is checked unconditionally, so the authoritative report of a failure is never the one that gets missed. Both directions are tested.subscribe'sTimeoutErrorcatch is scoped to the iteration alone, not to the callback. A progress callback that makes its own HTTP call can raiseTimeoutErrorfor reasons that have nothing to do with this wait, and cancelling a healthy queued request on the strength of it would throw a charge away. Tested.Verification of the pre-release path
The release-candidate publish is a human step, but the tooling half is checkable and worth knowing before the tag is cut:
publish.ymlvalidates the release tag as SemVer, where a pre-release is a--separated segment. Measured against that check —v0.2.0-rc1v0.2.0-rc.1v0.2.0rc1and building with the accepted form produces
comfy_sdk-0.2.0rc1-py3-none-any.whl,twine checkPASSED — the version normalises to PEP 440 on the way through, sopip install comfy-sdkwill not pick it up without--pre, which is the point of shipping one. The README's Releases section now says all of this.Provenance
uv run --extra dev pytest -q: 774 passed, 4 skipped, 0 failed (53 of them new, intests/test_models_queue.py);ruff check .clean;ruff format --check .: 53 files already formatted;mypy src: no issues in 20 files;scripts/check_public_repo_hygiene.py: no internal-only references;scripts/check_drift.py: models in sync, all 15 router error types covered, run path matches the vendored spec; a localpython -m buildof the tree at version0.2.0-rc1→comfy_sdk-0.2.0rc1-py3-none-any.whl,twine checkPASSED.submit_async/subscribe_asyncaliases in the plan are deliberately not implemented (reason above); the release-candidate publish is not done here (it is a human step — see Residual).Residual
Not fixed by this PR, and actionable on its own:
spec/router-openapi.yamland there was nothing to compare against; the queued surface is also gated server-side, so there was no deployment I could exercise it against from here. Everything in this PR is proven against the recorded stub intests/conftest.py, which is a stub I wrote to my own assumption of the shape — it cannot falsify that assumption. When the operations land in the vendored spec, reconcile the_MODEL_REQUEST*constants insrc/comfy_low/transport.pyagainst it and extendtests/test_router_spec_contract.pyto pin them the way it pins the run path (test_run_path_matches_vendored_speccurrently asserts the spec declares exactly one POST path, so it will need to admit the new ones). If the real routes differ, the four constants and the stub's four regexes are the whole change; the handle and the namespace methods above them are route-agnostic.request_id,status,queue_position,error_type,detail, andCOMPLETEDas the one terminal status. Each is parsed defensively so a mismatch degrades rather than crashes — but a wrong field name degrades silently into "the queue never completes" (an unknown status is treated as not-yet-terminal) rather than into an error. Worth an explicit check against the contract in the same pass as item 1.request_idcontaining/is refused (ComfyError, at submit, rather than at the first poll). Under the route shape above a request id is a single path segment, so this cannot arise — but it is a guard written against my own assumption, and it belongs in the item-1 reconciliation. It follows the discipline the shippedparse_model_idalready applies to the model id's segments.twine checked locally. To ship it: merge this, then create a GitHub Release taggedv0.2.0-rc1(notv0.2.0rc1). A plain release rather than a pre-release would not satisfy the ticket even though the code would be identical.Summary by CodeRabbit
New Features
Documentation
🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.