[PR #202] [MERGED] feat(models): add models.list() and models.schema() for Router model discovery #203

New issue

Closed

opened 2026-10-31 22:02:17 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:02:17 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/202
Author: @mattmillerai
Created: 9/29/2026
Status: ✅ Merged
Merged: 10/3/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-17771-models-list-schema

📝 Commits (4)

90c72f6 feat(models): add models.list() and models.schema() for Router model discovery
af766b5 fix(models): harden discovery parsing per review
3ceef4e Merge origin/main into matt/be-17771-models-list-schema
cd65888 docs(transport): drop the etag clause from post_model_run/post_model_submit docstrings

📊 Changes

10 files changed (+1402 additions, -9 deletions)

View changed files

📝 CHANGELOG.md (+12 -0)
📝 README.md (+81 -6)
📝 src/comfy_low/transport.py (+161 -0)
📝 src/comfy_sdk/__init__.py (+7 -0)
➕ src/comfy_sdk/model_catalog.py (+388 -0)
📝 src/comfy_sdk/models.py (+100 -0)
📝 tests/conftest.py (+97 -1)
➕ tests/test_models_discovery.py (+498 -0)
📝 tests/test_router_spec_contract.py (+54 -2)
📝 tests/test_sync_async_parity.py (+4 -0)

📄 Description

ELI-5

The Python SDK could run a Comfy Router model but couldn't tell you which models exist or what inputs a model takes. You had to write the HTTP calls yourself. This adds client.models.list() (walk the model catalog) and client.models.schema(model) (fetch a model's input/output OpenAPI document). Both work the same way as in the TypeScript SDK.

What changed

models.list(*, cursor=None, limit=None, timeout=DISCOVERY_TIMEOUT) calls GET {router_base_url}/v2/models. It returns a lazy ModelList:
- Iterating it walks every page, following next_cursor while has_more is true, and yields CatalogModel(id, provider, model, billing).
- .page() returns one ModelPage(data, has_more, next_cursor, limit, request_id).
- limit is sent unchanged, including values above 100, because the server clamps them.
- AsyncModels.list() returns an AsyncModelList (async for, await ….page()).
models.schema(model, *, etag=None, timeout=DISCOVERY_TIMEOUT) calls GET {router_base_url}/v2/models/{provider}/{model}/openapi.json. Each segment goes through the existing parse_model_id and is percent-encoded. It returns SchemaResult(unchanged, document, etag, request_id).
- With etag=, it sends If-None-Match, and a 304 returns unchanged=True, document=None rather than raising.
Behaviour shared with run(): Router base URL, bearer credential, retry policy (a 429 with Retry-After is ridden out), and error translation. Because a discovery read is a keyless GET that bills nothing, it also retries a 5xx or read timeout (the possibly-in-flight class a run must opt into) whenever the client's policy retries at all. NO_RETRY is still one attempt. 401/403/404/5xx raise the typed Router exception read from X-Comfy-Error-Type (Unauthorized, Forbidden, ModelNotFound, InternalError, ServiceUnavailable, …).
Transport (comfy_low): get_model_catalog / get_model_schema on both transports. The routes are confined to _MODEL_CATALOG_PATH / _MODEL_SCHEMA_PATH_TEMPLATE, and DISCOVERY_TIMEOUT = httpx.Timeout(30.0).
Tests:
- tests/test_router_spec_contract.py pins both routes by get.operationId (listRouterModels, getRouterModelInputSchema) against the vendored spec, plus the catalog's cursor/limit parameter names. A mutation check (moving the constant to /v3/models) fails the test.
- New tests/test_models_discovery.py (54 tests) drives the stub server. It covers:
  - a multi-page walk, sync and async
  - limit 101/500 passed through
  - a 304 on an etag match, sync and async, and a stale tag getting the new document
  - 404 → ModelNotFound, sync and async
  - typed 401/403/500/503 errors
  - a separate Router origin receiving the credential
  - retry, the 30 s default, and a per-call timeout
  - malformed ids rejected before any request
  - loop guards
  - malformed answers (a non-object page or schema 200, an entry whose id doesn't parse or doesn't match its provider/model) raising invalid_response
  - next_cursor cleared on the last page, a 304 without ETag keeping the sent tag, and an empty or non-ASCII etag refused before any request
- conftest.py gains the two routes.
README gets models.list / models.schema sections with runnable examples, and the hand-fetch openapi.json instruction now points at models.schema. There's also a CHANGELOG entry.

Judgment calls

Timeout default: the proposed signature said timeout=None, but it also asked for a 30 s default. In this SDK timeout=None already means "wait indefinitely" (run()), so the default is DISCOVERY_TIMEOUT (30 s) and None keeps its existing meaning.
AsyncModels.list is not async def: it returns the async iterable directly, so async for m in client.models.list(): works without an extra await, matching the TypeScript shape. It is declared, with that reason, in _SYNC_ON_BOTH in tests/test_sync_async_parity.py, which enforces the add-await contract.
Loop guards: a page that says has_more but has no next_cursor, or repeats a cursor already followed, raises ComfyError(code="invalid_response") instead of looping forever.
Stray 304: a 304 to a request that sent no etag is raised rather than reported as "unchanged". A caller with no tag has no cached copy that could be current.
billing is carried as the decoded JSON mapping rather than a class, so new server fields reach callers without a release. id/provider/model are validated because they address run(): id must parse as a run id and equal {provider}/{model}, so allowlisting on entry.provider and then running entry.id is safe.
None means only 304: the transport raises invalid_response for a 200 schema body that isn't a JSON object, so JSON null can't be read as "unchanged".
Hashing: CatalogModel hashes on its id triple (so set(client.models.list()) works). SchemaResult stays unhashable because its document is a mutable dict.
No page cap on the walk: the iterator is lazy, so the caller bounds it. A fixed ceiling could silently truncate a large catalog.
Test harness: the proposed approach mentioned an "httpx mock" pattern. The tests use the repo's stub server instead, because AGENTS.md says not to mock httpx.
ModelNotFound on the schema route is the server's own 404 answer, translated. The SDK doesn't deny any capability locally. Malformed ids are refused locally with ValueError, exactly as run() refuses them.

Verification against the live routes (read-only)

Unauthenticated curl of https://api.comfy.org/v2/models?limit=2 and …/v2/models/bfl/flux-2-pro/openapi.json both return 401 with X-Comfy-Error-Type: unauthorized and Router's error body. So both routes exist at the bound paths.
Through the SDK with a dummy key, client.models.list(limit=2).page() and client.models.schema("bfl/flux-2-pro") both raise Unauthorized (401, error_type="unauthorized", request_id populated).

Merged with `main`

Merged main at 3ceef4e to pick up #145 (binary run results). The conflicts were in the README and in src/comfy_sdk/models.py; both sides were kept. The "Two result shapes" section stays next to models.run, and its manual openapi.json fetch now links to models.schema. AsyncModels.list/schema sit before #145's module __all__. The credits_used spec-contract tripwire that failed on this branch earlier passes on the merged tree.

Residual

The success path against a real deployment is unverified. No valid credential was available, so these were checked only against the stub server and the live 401 answers:
- a 200 page of GET /v2/models
- a 200 document with ETag from GET /v2/models/{provider}/{model}/openapi.json
- a real 304 on If-None-Match
The README examples (client.models.list(), client.models.schema("bfl/flux-2-pro")) likewise weren't run end to end. Worth one authenticated smoke run, which is read-only and unbilled.
scripts/check_drift.py does not pin the two discovery routes. It still checks only the run route. The pytest contract test above does pin them and runs in CI's test job, but the codegen-drift job won't flag a moved discovery route for someone who only runs the drift script.
getRouterModel (GET /v2/models/{provider}/{model}) is still not exposed in Python (or TypeScript). It was out of scope by design; its only extra field, input_schema_url, is covered by schema().
TypeScript parity allowlist: the TypeScript SDK's surface-parity.test.ts lists modelsMethodsAheadOfPython: ["schema", "list"]. That entry should be removed in comfy-typescript-sdk once this merges.
Docs code samples: adding Python/TypeScript x-codeSamples to the Router OpenAPI spec for the docs reference pages is separate work in the upstream contract.
Stale docstring: the comfy_sdk/models.py module docstring still says "run is the one model operation today". That was already stale before this change (submit/subscribe/handle exist) and I didn't rewrite it here.

Provenance

Authored by: agent-work loop
Verified:
- At 3ceef4e (merge of main): ruff check . clean, ruff format --check . clean, mypy src 0 issues in 22 files, pytest 1135 passed / 9 skipped / 0 failed
- At 3ceef4e: scripts/check_drift.py all OK; scripts/check_public_repo_hygiene.py OK
- tests/test_models_discovery.py on Python 3.10 (at af766b5): 54 passed
- Live unauthenticated/dummy-key probe of both routes: 401 → typed Unauthorized
Deviations: default timeout is DISCOVERY_TIMEOUT (30 s) rather than None, so None keeps meaning "wait indefinitely". The success path wasn't exercised against a live deployment (no credential); see Residual.

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Browse the model catalog with synchronous or asynchronous clients, using cursor-based pagination or fetching a single page.
- Retrieve a model’s OpenAPI schema and use ETags to check whether it has changed.
- Discovery requests use the configured retry policy and provide typed errors.
Documentation
- Added guidance on catalog pagination, schema retrieval, and finding image input formats.

_{🔄 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/202 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/29/2026 **Status:** ✅ Merged **Merged:** 10/3/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-17771-models-list-schema` --- ### 📝 Commits (4) - [`90c72f6`](https://github.com/Comfy-Org/comfy-python-sdk/commit/90c72f6fcc529acc9c62408592fb38613f3837a3) feat(models): add models.list() and models.schema() for Router model discovery - [`af766b5`](https://github.com/Comfy-Org/comfy-python-sdk/commit/af766b5d277bc7031452945daa0a4069003033e7) fix(models): harden discovery parsing per review - [`3ceef4e`](https://github.com/Comfy-Org/comfy-python-sdk/commit/3ceef4ebacf508be25f8b404e358a2d56299309d) Merge origin/main into matt/be-17771-models-list-schema - [`cd65888`](https://github.com/Comfy-Org/comfy-python-sdk/commit/cd65888276b281d3384c5d3ac98cefa4be572b0d) docs(transport): drop the etag clause from post_model_run/post_model_submit docstrings ### 📊 Changes **10 files changed** (+1402 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+12 -0) 📝 `README.md` (+81 -6) 📝 `src/comfy_low/transport.py` (+161 -0) 📝 `src/comfy_sdk/__init__.py` (+7 -0) ➕ `src/comfy_sdk/model_catalog.py` (+388 -0) 📝 `src/comfy_sdk/models.py` (+100 -0) 📝 `tests/conftest.py` (+97 -1) ➕ `tests/test_models_discovery.py` (+498 -0) 📝 `tests/test_router_spec_contract.py` (+54 -2) 📝 `tests/test_sync_async_parity.py` (+4 -0) </details> ### 📄 Description ## ELI-5 The Python SDK could *run* a Comfy Router model but couldn't tell you *which* models exist or *what inputs* a model takes. You had to write the HTTP calls yourself. This adds `client.models.list()` (walk the model catalog) and `client.models.schema(model)` (fetch a model's input/output OpenAPI document). Both work the same way as in the TypeScript SDK. ## What changed - **`models.list(*, cursor=None, limit=None, timeout=DISCOVERY_TIMEOUT)`** calls `GET {router_base_url}/v2/models`. It returns a lazy `ModelList`: - Iterating it walks every page, following `next_cursor` while `has_more` is true, and yields `CatalogModel(id, provider, model, billing)`. - `.page()` returns one `ModelPage(data, has_more, next_cursor, limit, request_id)`. - `limit` is sent unchanged, including values above 100, because the server clamps them. - `AsyncModels.list()` returns an `AsyncModelList` (`async for`, `await ….page()`). - **`models.schema(model, *, etag=None, timeout=DISCOVERY_TIMEOUT)`** calls `GET {router_base_url}/v2/models/{provider}/{model}/openapi.json`. Each segment goes through the existing `parse_model_id` and is percent-encoded. It returns `SchemaResult(unchanged, document, etag, request_id)`. - With `etag=`, it sends `If-None-Match`, and a `304` returns `unchanged=True, document=None` rather than raising. - **Behaviour shared with `run()`:** Router base URL, bearer credential, retry policy (a `429` with `Retry-After` is ridden out), and error translation. Because a discovery read is a keyless GET that bills nothing, it also retries a `5xx` or read timeout (the possibly-in-flight class a run must opt into) whenever the client's policy retries at all. `NO_RETRY` is still one attempt. 401/403/404/5xx raise the typed Router exception read from `X-Comfy-Error-Type` (`Unauthorized`, `Forbidden`, `ModelNotFound`, `InternalError`, `ServiceUnavailable`, …). - **Transport (`comfy_low`):** `get_model_catalog` / `get_model_schema` on both transports. The routes are confined to `_MODEL_CATALOG_PATH` / `_MODEL_SCHEMA_PATH_TEMPLATE`, and `DISCOVERY_TIMEOUT = httpx.Timeout(30.0)`. - **Tests:** - `tests/test_router_spec_contract.py` pins both routes by `get.operationId` (`listRouterModels`, `getRouterModelInputSchema`) against the vendored spec, plus the catalog's `cursor`/`limit` parameter names. A mutation check (moving the constant to `/v3/models`) fails the test. - New `tests/test_models_discovery.py` (54 tests) drives the stub server. It covers: - a multi-page walk, sync and async - `limit` 101/500 passed through - a `304` on an `etag` match, sync and async, and a stale tag getting the new document - `404` → `ModelNotFound`, sync and async - typed 401/403/500/503 errors - a separate Router origin receiving the credential - retry, the 30 s default, and a per-call timeout - malformed ids rejected before any request - loop guards - malformed answers (a non-object page or schema `200`, an entry whose `id` doesn't parse or doesn't match its `provider`/`model`) raising `invalid_response` - `next_cursor` cleared on the last page, a `304` without `ETag` keeping the sent tag, and an empty or non-ASCII `etag` refused before any request - `conftest.py` gains the two routes. - README gets `models.list` / `models.schema` sections with runnable examples, and the hand-fetch `openapi.json` instruction now points at `models.schema`. There's also a CHANGELOG entry. ## Judgment calls - **Timeout default:** the proposed signature said `timeout=None`, but it also asked for a 30 s default. In this SDK `timeout=None` already means "wait indefinitely" (`run()`), so the default is `DISCOVERY_TIMEOUT` (30 s) and `None` keeps its existing meaning. - **`AsyncModels.list` is not `async def`:** it returns the async iterable directly, so `async for m in client.models.list():` works without an extra `await`, matching the TypeScript shape. It is declared, with that reason, in `_SYNC_ON_BOTH` in `tests/test_sync_async_parity.py`, which enforces the add-`await` contract. - **Loop guards:** a page that says `has_more` but has no `next_cursor`, or repeats a cursor already followed, raises `ComfyError(code="invalid_response")` instead of looping forever. - **Stray 304:** a `304` to a request that sent no `etag` is raised rather than reported as "unchanged". A caller with no tag has no cached copy that could be current. - **`billing`** is carried as the decoded JSON mapping rather than a class, so new server fields reach callers without a release. `id`/`provider`/`model` are validated because they address `run()`: `id` must parse as a run id and equal `{provider}/{model}`, so allowlisting on `entry.provider` and then running `entry.id` is safe. - **`None` means only 304:** the transport raises `invalid_response` for a `200` schema body that isn't a JSON object, so JSON `null` can't be read as "unchanged". - **Hashing:** `CatalogModel` hashes on its id triple (so `set(client.models.list())` works). `SchemaResult` stays unhashable because its `document` is a mutable dict. - **No page cap on the walk:** the iterator is lazy, so the caller bounds it. A fixed ceiling could silently truncate a large catalog. - **Test harness:** the proposed approach mentioned an "httpx mock" pattern. The tests use the repo's stub server instead, because `AGENTS.md` says not to mock `httpx`. - **`ModelNotFound` on the schema route** is the server's own `404` answer, translated. The SDK doesn't deny any capability locally. Malformed ids are refused locally with `ValueError`, exactly as `run()` refuses them. ## Verification against the live routes (read-only) - Unauthenticated `curl` of `https://api.comfy.org/v2/models?limit=2` and `…/v2/models/bfl/flux-2-pro/openapi.json` both return `401` with `X-Comfy-Error-Type: unauthorized` and Router's error body. So both routes exist at the bound paths. - Through the SDK with a dummy key, `client.models.list(limit=2).page()` and `client.models.schema("bfl/flux-2-pro")` both raise `Unauthorized` (401, `error_type="unauthorized"`, `request_id` populated). ## Merged with `main` Merged `main` at 3ceef4e to pick up #145 (binary run results). The conflicts were in the README and in `src/comfy_sdk/models.py`; both sides were kept. The "Two result shapes" section stays next to `models.run`, and its manual `openapi.json` fetch now links to `models.schema`. `AsyncModels.list`/`schema` sit before #145's module `__all__`. The `credits_used` spec-contract tripwire that failed on this branch earlier passes on the merged tree. ## Residual - **The success path against a real deployment is unverified.** No valid credential was available, so these were checked only against the stub server and the live `401` answers: - a `200` page of `GET /v2/models` - a `200` document with `ETag` from `GET /v2/models/{provider}/{model}/openapi.json` - a real `304` on `If-None-Match` The README examples (`client.models.list()`, `client.models.schema("bfl/flux-2-pro")`) likewise weren't run end to end. Worth one authenticated smoke run, which is read-only and unbilled. - **`scripts/check_drift.py` does not pin the two discovery routes.** It still checks only the run route. The pytest contract test above does pin them and runs in CI's `test` job, but the `codegen-drift` job won't flag a moved discovery route for someone who only runs the drift script. - **`getRouterModel` (`GET /v2/models/{provider}/{model}`)** is still not exposed in Python (or TypeScript). It was out of scope by design; its only extra field, `input_schema_url`, is covered by `schema()`. - **TypeScript parity allowlist:** the TypeScript SDK's `surface-parity.test.ts` lists `modelsMethodsAheadOfPython: ["schema", "list"]`. That entry should be removed in `comfy-typescript-sdk` once this merges. - **Docs code samples:** adding Python/TypeScript `x-codeSamples` to the Router OpenAPI spec for the docs reference pages is separate work in the upstream contract. - **Stale docstring:** the `comfy_sdk/models.py` module docstring still says "`run` is the one model operation today". That was already stale before this change (`submit`/`subscribe`/`handle` exist) and I didn't rewrite it here. ## Provenance - **Authored by:** agent-work loop - **Verified:** - At 3ceef4e (merge of `main`): `ruff check .` clean, `ruff format --check .` clean, `mypy src` 0 issues in 22 files, `pytest` 1135 passed / 9 skipped / 0 failed - At 3ceef4e: `scripts/check_drift.py` all OK; `scripts/check_public_repo_hygiene.py` OK - `tests/test_models_discovery.py` on Python 3.10 (at af766b5): 54 passed - Live unauthenticated/dummy-key probe of both routes: 401 → typed `Unauthorized` - **Deviations:** default `timeout` is `DISCOVERY_TIMEOUT` (30 s) rather than `None`, so `None` keeps meaning "wait indefinitely". The success path wasn't exercised against a live deployment (no credential); see Residual. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **New Features** * Browse the model catalog with synchronous or asynchronous clients, using cursor-based pagination or fetching a single page. * Retrieve a model’s OpenAPI schema and use ETags to check whether it has changed. * Discovery requests use the configured retry policy and provide typed errors. * **Documentation** * Added guidance on catalog pagination, schema retrieval, and finding image input formats.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:02:17 +00:00