[PR #73] [MERGED] feat: add models.run on both the sync and async clients #73

New issue

Closed

opened 2026-10-31 22:01:09 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:01:09 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/73
Author: @mattmillerai
Created: 8/23/2026
Status: ✅ Merged
Merged: 8/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8492-models-run

📝 Commits (1)

1e8774f feat: add models.run on both the sync and async clients

📊 Changes

6 files changed (+494 additions, -3 deletions)

View changed files

📝 CHANGELOG.md (+11 -1)
📝 README.md (+30 -0)
📝 src/comfy_low/transport.py (+84 -1)
📝 src/comfy_sdk/models.py (+77 -1)
📝 tests/conftest.py (+51 -0)
➕ tests/test_models_run.py (+241 -0)

📄 Description

ELI-5

You give the client a model name and a bag of arguments; you get the finished picture (or video, or whatever the model makes) back from that one call. Nothing to poll, nothing to wait on yourself. If you're writing async code, you use the async client and put await in front of it — that's the whole difference. There is no second, differently-named method for the async case, and there never will be.

What changed

client.models.run(model, arguments) on both Comfy and AsyncComfy (src/comfy_sdk/models.py). One call, returns the result of a completed generation. The sync one blocks; the async one is awaited and returns the same shape.
The awaitable form is the async client, not a suffix. No run_async() exists on either client, either namespace, or either transport — and that is asserted by tests over all six classes rather than left to absence, plus a broader guard that no public method on those classes may encode sync-vs-async in its name (^async_ / _async$ / _sync$). aclose is the one deliberate rename and does not match; the existing parity test already covers it.
A run-sized default timeout — MODEL_RUN_TIMEOUT (src/comfy_low/transport.py), 10 minutes read/write/pool with a 10s connect. The server holds the connection until the generation is complete, polling the upstream provider itself where the provider is submit/poll, so the client's own 30s default would abort a perfectly healthy run. connect stays short: an unreachable host is not a slow generation. Callers can pass seconds, an httpx.Timeout, or None to wait indefinitely.
Idempotency-Key plumbed onto every run. A fresh key is minted per call unless idempotency_key= is supplied, matching what submit() already does. The value semantics and the retry interaction are deliberately not decided here.
Transport binding post_model_run on ComfyLow and AsyncComfyLow, returning the decoded body verbatim.
Tests — tests/test_models_run.py (35 tests) against the stubbed server, plus a POST /api/v2/models/run route in tests/conftest.py.
Docs — a ### models.run subsection in the README's models-namespace section, a CHANGELOG [Unreleased] entry, and module/method docstrings.

The one real judgment call: the wire contract is not published yet

spec/openapi.yaml (the vendored, one-way-synced v2 contract, spec/VERSION 2.0.0) declares 11 operationIds, 0 of which are model routes — the same measurement the namespace PR reported, unchanged since. So this method could not be typed against or generated from a committed contract, and the request shape had to be chosen rather than read. What I chose, and why:

POST /api/v2/models/run with {"model": ..., "arguments": {...}} in the body, rather than the model id in the path. Model ids are commonly provider-namespaced and contain / (vendor/family/variant); in a path segment that needs percent-encoding which intermediaries normalize inconsistently. A named body field also matches how the rest of this API shapes request bodies ({"workflow": ...} on job submission) and leaves room for sibling fields without moving the route.
Contained on purpose. The entire wire shape lives in one constant and one sans-IO function — _MODEL_RUN_PATH and model_run_request() in src/comfy_low/transport.py. Reconciling it when the real route is vendored is a change to those two things; no caller-facing signature, no test outside the request-shape assertions, and no other transport method depends on it.
Kept out of OPERATION_IDS. The spec-coverage test asserts that set equals the spec's exactly, so a non-spec binding must not be registered there. Both the module docstring and the constant say plainly that this one binding is not backed by an operationId and why.

If the router's published route differs, that is a one-place fix — but it is a genuine unknown and I would not merge this ahead of the server-side contract landing without someone confirming the shape.

Other judgment calls

arguments is a required positional parameter, not one defaulting to {}. Adding a default later is backwards compatible; removing one is not, and the ticket's own framing is that a published signature cannot be withdrawn. A model that takes nothing gets {}.
Success is 200 or 201; anything else raises. A 202 Accepted would mean the server did not await completion, which contradicts the contract this method implements — returning an unfinished payload as if it were a result would be worse than raising. If the router really does answer 202 for some class of model, that is a contract question, not a client patch.
No 429/retry handling here. Comfy.submit/Comfy.run retry a 429 carrying Retry-After; models.run does not, so a queue_full surfaces immediately as QueueFull. That asymmetry is deliberate — retry policy is explicitly separate work — but it is the thing a reviewer is most likely to read as an omission, so: it is one.
Errors are translated to the SDK's existing exceptions via the existing translating() helper, so a protocol ApiError never reaches an integrator (the rule PR #63 established). No new exception types were added; that taxonomy is separate work. An unmapped code lands as ComfyError with its code and http_status intact, which a test asserts.
The return annotation is dict[str, Any] — the payload contract is a JSON object. A hypothetical top-level JSON array would pass through unvalidated (the annotation would then be imprecise). I did not add a shape guard: the promise is to hand back the provider's payload untouched, and rejecting a valid-but-unexpected body would be inventing a failure the server has not specified.
cast() rather than restructuring _ModelsBase. _low is typed as the sync-or-async union on the shared base (narrowing it per subclass is a mypy override error, as the namespace PR noted). Each run casts locally instead. Making _ModelsBase generic would remove the casts, but it is a type-level refactor of just-merged code and outside what this change needs.
client.run(workflow) and client.models.run(model, arguments) now sit side by side and are different operations — one runs a workflow graph, the other runs a hosted model. I left client.run completely alone: renaming a published method is breaking and is nobody's call to make in this diff. Worth a docs pass if the collision reads badly in practice.
One shared-test-infrastructure change: the stub server's handler now swallows BrokenPipeError / ConnectionResetError in handle_one_request. The timeout tests deliberately abort a slow request, which previously printed a traceback into the test output. Only those two exception types are caught, and neither could ever fail a test before (they were logged, not raised into the test), so nothing else changes.

Negative-claim falsification

This diff adds a capability; it denies none. There is no "not supported" branch, no stub that raises, no test flipped to assert a dead-end. Two strings in the diff would trip a mechanical denial scan, and both are in the same test: model_unavailable appears as a server-sent error code in the stub, used to prove the SDK passes an unmapped code through with its code and http_status intact instead of swallowing or renaming it. Nothing in src/ ever produces that message. The positive capability is exercised directly and passes on both clients (run returns a completed result, sync and async, against the stub). The only raises this change can produce are (i) pass-through of an error envelope the server sent and (ii) an httpx timeout when the caller explicitly configures one shorter than the run — which has its own control test proving the default is what prevents it.

Sizing the part not covered

A sweep of every class defined in comfy_sdk and comfy_low — 639 public attributes across both packages — found 0 existing names matching the suffixed-async pattern this change now guards against, so the guard codifies the current state rather than cleaning up after it. On the contract side, the 11 declared operationIds, 0 of them model routes above is the measurement of what is still unspecified.

Not exercised here

The specification, the design decision record, and the tracker issues that settle this method's signature and its one-async-mechanism rule live in internal tools that are not reachable from the environment this was built in; the requirements were taken as given rather than re-read at the source. The related server-side story that makes a submit/poll provider resolve inside a single call is likewise named but unread — this change depends on its contract (the server does not answer until the run is complete), which is exactly the assumption the timeout default and the tests encode, but I could not read that story to confirm it. There is no live router endpoint to exercise from here, so nothing in this change has been run against a real model run: verification is the stubbed server plus the full local gate set, below.

Provenance

Authored by: agent-work loop
Verified: pytest 215 passed / 4 skipped (was 180/4 on the base — 35 new); mypy src no issues in 17 files; ruff check . clean; ruff format --check . clean (45 files); scripts/check_drift.py models in sync with the spec; scripts/check_public_repo_hygiene.py clean
Deviations: the request path and body shape are a chosen contract, not a read one (see the judgment-call section — the vendored spec declares no model routes); no retry policy, no new exception types, and no parity-introspection test, all of which are separate work

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-python-sdk/pull/73 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/23/2026 **Status:** ✅ Merged **Merged:** 8/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8492-models-run` --- ### 📝 Commits (1) - [`1e8774f`](https://github.com/Comfy-Org/comfy-python-sdk/commit/1e8774fdae314fb18a0102f8c4ca3710178d0927) feat: add models.run on both the sync and async clients ### 📊 Changes **6 files changed** (+494 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+11 -1) 📝 `README.md` (+30 -0) 📝 `src/comfy_low/transport.py` (+84 -1) 📝 `src/comfy_sdk/models.py` (+77 -1) 📝 `tests/conftest.py` (+51 -0) ➕ `tests/test_models_run.py` (+241 -0) </details> ### 📄 Description ## ELI-5 You give the client a model name and a bag of arguments; you get the finished picture (or video, or whatever the model makes) back from that one call. Nothing to poll, nothing to wait on yourself. If you're writing async code, you use the async client and put `await` in front of it — that's the whole difference. There is no second, differently-named method for the async case, and there never will be. ## What changed - **`client.models.run(model, arguments)` on both `Comfy` and `AsyncComfy`** (`src/comfy_sdk/models.py`). One call, returns the result of a completed generation. The sync one blocks; the async one is awaited and returns the same shape. - **The awaitable form is the async client, not a suffix.** No `run_async()` exists on either client, either namespace, or either transport — and that is asserted by tests over all six classes rather than left to absence, plus a broader guard that no public method on those classes may encode sync-vs-async in its *name* (`^async_` / `_async$` / `_sync$`). `aclose` is the one deliberate rename and does not match; the existing parity test already covers it. - **A run-sized default timeout** — `MODEL_RUN_TIMEOUT` (`src/comfy_low/transport.py`), 10 minutes read/write/pool with a 10s connect. The server holds the connection until the generation is complete, polling the upstream provider itself where the provider is submit/poll, so the client's own 30s default would abort a perfectly healthy run. `connect` stays short: an unreachable host is not a slow generation. Callers can pass seconds, an `httpx.Timeout`, or `None` to wait indefinitely. - **`Idempotency-Key` plumbed onto every run.** A fresh key is minted per call unless `idempotency_key=` is supplied, matching what `submit()` already does. The value semantics and the retry interaction are deliberately not decided here. - **Transport binding** `post_model_run` on `ComfyLow` and `AsyncComfyLow`, returning the decoded body verbatim. - **Tests** — `tests/test_models_run.py` (35 tests) against the stubbed server, plus a `POST /api/v2/models/run` route in `tests/conftest.py`. - **Docs** — a `### models.run` subsection in the README's models-namespace section, a CHANGELOG `[Unreleased]` entry, and module/method docstrings. ## The one real judgment call: the wire contract is not published yet `spec/openapi.yaml` (the vendored, one-way-synced v2 contract, `spec/VERSION` 2.0.0) declares **11 operationIds, 0 of which are model routes** — the same measurement the namespace PR reported, unchanged since. So this method could not be typed against or generated from a committed contract, and the request shape had to be chosen rather than read. What I chose, and why: - **`POST /api/v2/models/run` with `{"model": ..., "arguments": {...}}` in the body**, rather than the model id in the path. Model ids are commonly provider-namespaced and contain `/` (`vendor/family/variant`); in a path segment that needs percent-encoding which intermediaries normalize inconsistently. A named body field also matches how the rest of this API shapes request bodies (`{"workflow": ...}` on job submission) and leaves room for sibling fields without moving the route. - **Contained on purpose.** The entire wire shape lives in one constant and one sans-IO function — `_MODEL_RUN_PATH` and `model_run_request()` in `src/comfy_low/transport.py`. Reconciling it when the real route is vendored is a change to those two things; no caller-facing signature, no test outside the request-shape assertions, and no other transport method depends on it. - **Kept out of `OPERATION_IDS`.** The spec-coverage test asserts that set equals the spec's exactly, so a non-spec binding must not be registered there. Both the module docstring and the constant say plainly that this one binding is not backed by an `operationId` and why. If the router's published route differs, that is a one-place fix — but it is a genuine unknown and I would not merge this ahead of the server-side contract landing without someone confirming the shape. ## Other judgment calls 1. **`arguments` is a required positional parameter**, not one defaulting to `{}`. Adding a default later is backwards compatible; removing one is not, and the ticket's own framing is that a published signature cannot be withdrawn. A model that takes nothing gets `{}`. 2. **Success is `200` or `201`; anything else raises.** A `202 Accepted` would mean the server did *not* await completion, which contradicts the contract this method implements — returning an unfinished payload as if it were a result would be worse than raising. If the router really does answer 202 for some class of model, that is a contract question, not a client patch. 3. **No 429/retry handling here.** `Comfy.submit`/`Comfy.run` retry a 429 carrying `Retry-After`; `models.run` does not, so a `queue_full` surfaces immediately as `QueueFull`. That asymmetry is deliberate — retry policy is explicitly separate work — but it is the thing a reviewer is most likely to read as an omission, so: it is one. 4. **Errors are translated to the SDK's existing exceptions** via the existing `translating()` helper, so a protocol `ApiError` never reaches an integrator (the rule PR #63 established). No new exception types were added; that taxonomy is separate work. An unmapped code lands as `ComfyError` with its `code` and `http_status` intact, which a test asserts. 5. **The return annotation is `dict[str, Any]`** — the payload contract is a JSON object. A hypothetical top-level JSON array would pass through unvalidated (the annotation would then be imprecise). I did not add a shape guard: the promise is to hand back the provider's payload untouched, and rejecting a valid-but-unexpected body would be inventing a failure the server has not specified. 6. **`cast()` rather than restructuring `_ModelsBase`.** `_low` is typed as the sync-or-async union on the shared base (narrowing it per subclass is a mypy override error, as the namespace PR noted). Each `run` casts locally instead. Making `_ModelsBase` generic would remove the casts, but it is a type-level refactor of just-merged code and outside what this change needs. 7. **`client.run(workflow)` and `client.models.run(model, arguments)` now sit side by side** and are different operations — one runs a workflow graph, the other runs a hosted model. I left `client.run` completely alone: renaming a published method is breaking and is nobody's call to make in this diff. Worth a docs pass if the collision reads badly in practice. 8. **One shared-test-infrastructure change:** the stub server's handler now swallows `BrokenPipeError` / `ConnectionResetError` in `handle_one_request`. The timeout tests deliberately abort a slow request, which previously printed a traceback into the test output. Only those two exception types are caught, and neither could ever fail a test before (they were logged, not raised into the test), so nothing else changes. ## Negative-claim falsification This diff **adds** a capability; it denies none. There is no "not supported" branch, no stub that raises, no test flipped to assert a dead-end. Two strings in the diff would trip a mechanical denial scan, and both are in the same test: `model_unavailable` appears as a *server-sent* error code in the stub, used to prove the SDK passes an unmapped code through with its `code` and `http_status` intact instead of swallowing or renaming it. Nothing in `src/` ever produces that message. The positive capability is exercised directly and passes on both clients (`run` returns a completed result, sync and async, against the stub). The only raises this change can produce are (i) pass-through of an error envelope the server sent and (ii) an `httpx` timeout when the caller explicitly configures one shorter than the run — which has its own control test proving the default is what prevents it. ## Sizing the part not covered A sweep of every class defined in `comfy_sdk` and `comfy_low` — **639 public attributes across both packages** — found **0** existing names matching the suffixed-async pattern this change now guards against, so the guard codifies the current state rather than cleaning up after it. On the contract side, the **11 declared operationIds, 0 of them model routes** above is the measurement of what is still unspecified. ## Not exercised here The specification, the design decision record, and the tracker issues that settle this method's signature and its one-async-mechanism rule live in internal tools that are not reachable from the environment this was built in; the requirements were taken as given rather than re-read at the source. The related server-side story that makes a submit/poll provider resolve inside a single call is likewise named but unread — this change depends on its *contract* (the server does not answer until the run is complete), which is exactly the assumption the timeout default and the tests encode, but I could not read that story to confirm it. **There is no live router endpoint to exercise from here**, so nothing in this change has been run against a real model run: verification is the stubbed server plus the full local gate set, below. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pytest` 215 passed / 4 skipped (was 180/4 on the base — 35 new); `mypy src` no issues in 17 files; `ruff check .` clean; `ruff format --check .` clean (45 files); `scripts/check_drift.py` models in sync with the spec; `scripts/check_public_repo_hygiene.py` clean - **Deviations:** the request path and body shape are a chosen contract, not a read one (see the judgment-call section — the vendored spec declares no model routes); no retry policy, no new exception types, and no parity-introspection test, all of which are separate work --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:01:09 +00:00