diff --git a/CHANGELOG.md b/CHANGELOG.md index d28a4a5..455d2b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,9 +56,73 @@ the fuller account of each version, including verification notes. control characters, ANSI escapes and bidi overrides reduced, whitespace collapsed, and a 256-character cap — so a hostile or merely careless `msg` can no longer scribble on a terminal or flood a log line. Only the summary string changes; `.errors` still carries the raw typed entries. +- `client.models.run()` no longer throws away a generation whose model answers + in bytes rather than JSON. Comfy Router forwards a partner model's output + under the partner's *own* media type, and for a model whose partner returns a + generation directly as a file — the ElevenLabs audio models are the first of + these in the catalog — that is raw `audio/mpeg`. The SDK called `.json()` on + every 2xx regardless, so such a run raised `ComfyError` with + `code="invalid_response"` (`UnicodeDecodeError: 'utf-8' codec can't decode + byte 0xff`, the MP3 frame sync) *after* the generation had run and been + billed. Those models were unusable from this SDK. -### Changed +### Added +- `BinaryResult` — importable from `comfy_sdk` — the second shape + `models.run()` can return. `run` now branches on the response + `Content-Type`, exactly as the run route's published `200` says a client + must: `application/json` (or a `+json` suffix type) decodes to a `dict` + exactly as before, and anything else comes back as + `BinaryResult(content, content_type, request_id)`. The bytes are the + partner's file verbatim — not base64-encoded, not wrapped in a dict, not + decoded or transcoded — so `Path("out.mp3").write_bytes(result.content)` is + the whole of it. `content_type` is the header including its parameters, + because for some partner media types the parameters are part of what the + bytes are (`audio/L16; rate=16000`); it is bounded and stripped of + unprintable characters first, which no real media type contains, the way + every other server-supplied string this SDK surfaces already is. The return + annotation is therefore `dict[str, Any] | BinaryResult`; a caller that only + uses JSON models sees no behaviour change, but a type checker will now ask + them to narrow. `run_detailed` is the same story one level out: + `RouterRunResult.output` carries whichever of the two shapes the run + answered with. + + Two boundaries worth knowing: a 2xx whose `Content-Type` claims JSON and + whose body will not parse still raises `invalid_response` (there the response + promised a document and did not deliver one), while a 2xx that names *no* + `Content-Type` is a `BinaryResult` unless its body is empty (`{}`, as on + every other operation) or parses as a JSON **object**. Object, not merely + valid JSON: that branch probes bytes nothing declared, so `null`, `[...]` and + a bare number are bytes — accepting one would return a value outside the + declared union, and a short binary body of all-ASCII digits parses as a + number. The binary path runs inside the same translation as the JSON one, so + a failure still carries `.idempotency_key` and an `Idempotent-Replayed` + binary 200 comes back like a first run. + + The one deliberate behaviour change beyond the fix: a non-JSON 2xx used to be + read as "a proxy interstitial served as 200" and raised. On this route that + reading is no longer available — the SDK cannot tell an interstitial from a + partner's native text output, and the contract says the body is the + partner's — so a `text/html` 200 now reaches the caller as bytes they can + inspect, rather than discarding a generation they were billed for. Every + other operation keeps the old reading, because JSON is the only success media + type their routes declare. The same asymmetry decides the empty case: a + declared-binary 200 with a zero-length body is a `BinaryResult` holding no + bytes rather than an exception. Two checks tell an answer from an artefact — + `request_id is None` means no Router answer was seen at all (the header is + required on every one Router sends), and `not content` means nothing was + delivered. + + The queued surface gets the identical branch: `RequestHandle.get()` / + `AsyncRequestHandle.get()` and `models.subscribe()` now return + `dict[str, Any] | BinaryResult` too, because the result route they collect + from (`GET .../requests/{request_id}`) declares the same `application/json` / + `*/*` pair `models.run()` does. Before this it still went through the + JSON-only decoder, so a binary generation submitted through `submit()` raised + `invalid_response` on collection even though the identical model run directly + through `run()` already worked. + +### Changed - **Because those three buckets are now one class each, they descend from `RouterError` on the workflow surface too**: a `POST /jobs` call that fails `401`/`403`/`402` raises a `RouterError` subclass. `except Unauthorized` / `except Forbidden` / `except InsufficientCredits` (from either diff --git a/README.md b/README.md index f3daeda..9a83981 100644 --- a/README.md +++ b/README.md @@ -433,8 +433,12 @@ Three things follow from that, and they are the whole contract of this method: `run` returns when the generation is **complete**. There is no submit step and nothing to poll: where the platform has to submit-and-poll an upstream provider, that happens server side inside this one call. The value you get back -is the provider's own payload — decoded JSON, handed over as-is, with no -wrapper class between you and the fields the provider documented. +is the provider's own payload, handed over as-is. For a model that answers +JSON — most of them — that is a `dict` with no wrapper class between you and +what the provider produced. For a model whose partner answers a generation +directly as bytes, it is a `BinaryResult` carrying those bytes unchanged +alongside the `content_type` and `request_id` that came with them; see "Two +result shapes" below. The awaitable form is the **async client**, not a differently-named method: @@ -446,6 +450,62 @@ async with AsyncComfy(api_key="comfyui-...") as client: There is no `run_async()`, and there will not be one — one operation, one name, and `await` is what makes it asynchronous. +### Two result shapes — JSON, or the model's own bytes + +Router forwards the partner's output *under the partner's own media type*, so +`run` returns one of two things, decided by the response's `Content-Type`: + +| The model answers with | You get back | Read it as | +|---|---|---| +| a JSON document (`application/json`, or a `+json` type) | `dict` | `result["images"][0]["url"]` | +| raw bytes under its own media type (`audio/mpeg`, ...) | `BinaryResult` | `result.content`, `result.content_type`, `result.request_id` | + +Almost every model in the catalog is the first row, and that shape is unchanged. +The second row is for a model whose partner answers a generation *directly as a +file* — the ElevenLabs audio models are the first of these. The bytes come back +exactly as they arrived: not base64-encoded, not wrapped in a dict, not decoded +or transcoded. Write them to a file and you have the file the partner produced: + +```python +from pathlib import Path + +from comfy_sdk import BinaryResult, Comfy + +client = Comfy(api_key="comfyui-...") +result = client.models.run( + "elevenlabs/eleven_v3", + {"inputs": [{"text": "Hello from Comfy Router.", "voice_id": "..."}]}, +) + +assert isinstance(result, BinaryResult) +print(result.content_type) # 'audio/mpeg' +print(result.request_id) # the server's X-Comfy-Request-Id, or None +Path("hello.mp3").write_bytes(result.content) +``` + +`BinaryResult` is importable from `comfy_sdk` for exactly this `isinstance` +check. Which shape a given model returns is in its own contract — `GET +/v2/models/{provider}/{model}/openapi.json`, whose `200` is `application/json` +for a JSON model and `*/*` with `format: binary` for a bytes one. + +Note `content_type` keeps the header's **parameters**, because for some media +types the parameters are part of what the bytes are — ElevenLabs' `pcm_*` output +formats come back as `audio/L16; rate=16000`, and the sample rate is not +decoration. It is bounded and stripped of unprintable characters first, the way +every other server-supplied string this SDK hands you is; no real media type +contains either, so what you get is what was sent. And a `200` whose +`Content-Type` *claims* JSON but whose body will not parse is still an error +(`ComfyError`, `code="invalid_response"`), not bytes: there the response +promised a document and did not deliver one. + +A non-JSON `200` reaches you even when it is an intermediary's error page or an +empty body — this route's `200` means a generation ran and was billed, so the +SDK will not destroy one it merely finds suspicious. Two checks tell you: +`result.request_id is None` means no Router answer was seen at all (Router's +contract marks that header required on every answer it sends, so an HTML +interstitial from a proxy in front of it has none), and `not result.content` +means nothing was delivered. Check them before writing `content` to disk. + ### Image to image — upload an asset first An image-to-image model takes an image *as input*, and Router forwards the @@ -535,7 +595,7 @@ The handle carries four operations: | | | |---|---| | `handle.status()` | one authoritative poll, returned as a `QueueUpdate` (`status`, `queue_position`, `error_type`, `retry_after`, `raw`) | -| `handle.get(timeout=None)` | poll to completion, then return the provider's own payload — the same value `run` would have returned | +| `handle.get(timeout=None)` | poll to completion, then return the provider's own payload — the same value `run` would have returned, including the `BinaryResult` branch from "Two result shapes" above | | `handle.cancel()` | ask the server to cancel. A request, not a guarantee: a request that already completed stays completed | | `handle.iter_events(timeout=None)` | the poll loop with its updates exposed — yields the first observation, every change of status or queue position, and the completion | diff --git a/scripts/check_drift.py b/scripts/check_drift.py index 256110f..66a5da6 100755 --- a/scripts/check_drift.py +++ b/scripts/check_drift.py @@ -25,7 +25,11 @@ and ``servers[0].url``. A sync that *moves* the route (the ``/v1`` -> ``/v2`` move already on the roadmap) while those constants stay put would leave the SDK posting to a route the contract no longer declares, with nothing else in - CI noticing. + CI noticing. The same check covers the **media types that route's 200 can + answer under**, because the SDK hand-branches on them too: an + ``application/json`` success decodes to a ``dict`` and any other one comes + back as a ``BinaryResult``, so a branch added or dropped upstream changes + what ``post_model_run`` returns. ``tests/test_router_spec_contract.py`` asserts the same things from the test suite. Both exist on purpose: the suite is where a contributor sees it, and @@ -44,6 +48,15 @@ COMMITTED = ROOT / "src" / "comfy_low" / "models" / "_generated.py" ROUTER_SPEC = ROOT / "spec" / "router-openapi.yaml" +#: The media types ``runRouterModel``'s ``200`` is expected to declare, sorted. +#: This is not a restatement of the spec for its own sake: it is the shape +#: ``comfy_low.transport._Prepared.parse_run_result`` is written against -- one +#: JSON branch that decodes to a ``dict``, one catch-all binary branch that +#: comes back as a ``BinaryResult``. Unlike the path and host below, there is no +#: constant in the SDK to compare against, because the branch is control flow +#: rather than data; so the expectation lives here. +_BOUND_RUN_MEDIA_TYPES = ["*/*", "application/json"] + def _generate(out: Path) -> None: subprocess.run( @@ -169,8 +182,8 @@ def _declared_router_error_types() -> list[dict[str, str]]: return entries -def _declared_run_route() -> tuple[str, str]: - """The spec's ``(runRouterModel path, servers[0].url)``. +def _declared_run_route() -> tuple[str, str, list[str]]: + """The spec's ``(runRouterModel path, servers[0].url, 200 media types)``. Same failure policy as :func:`_declared_router_error_types`: every way the file can be unusable becomes a ``ValueError`` with a sentence someone can @@ -217,7 +230,25 @@ def _declared_run_route() -> tuple[str, str]: host = servers[0].get("url") if not isinstance(host, str) or not host: raise ValueError(f"{ROUTER_SPEC.name}'s servers[0].url is not a non-empty string") - return declared[0], host + + # The media types that route's 200 can answer under. The SDK branches on + # exactly two -- JSON to a dict, anything else to a BinaryResult -- so a + # sync that drops or adds one changes what `post_model_run` must return. + responses = paths[declared[0]]["post"].get("responses") + # `"200"` *or* `200`: the vendored spec quotes its status codes, but PyYAML + # reads an unquoted `200:` as the integer, and a sync from a generator that + # does not quote them would otherwise fail this gate with "declares no 200 + # response" -- a message about the wrong thing entirely. `_declared_router_ + # error_types` is defensive about its own shape for the same reason. + ok = responses.get("200") if isinstance(responses, dict) else None + if not isinstance(ok, dict) and isinstance(responses, dict): + ok = responses.get(200) + if not isinstance(ok, dict): + raise ValueError(f"{ROUTER_SPEC.name}'s runRouterModel declares no 200 response") + content = ok.get("content") + if not isinstance(content, dict) or not content: + raise ValueError(f"{ROUTER_SPEC.name}'s runRouterModel 200 declares no content") + return declared[0], host, sorted(content) def _check_router_run_route() -> int: @@ -232,12 +263,24 @@ def _check_router_run_route() -> int: return 1 try: - declared_path, declared_host = _declared_run_route() + declared_path, declared_host, declared_media = _declared_run_route() except ValueError as exc: print(f"ERROR: {exc}", file=sys.stderr) return 1 failed = False + if declared_media != _BOUND_RUN_MEDIA_TYPES: + print( + f"ERROR: the model-run 200's media types have drifted from {ROUTER_SPEC.name}.\n" + f" spec (runRouterModel 200 content): {declared_media}\n" + f" sdk (the branches parse_run_result takes): {_BOUND_RUN_MEDIA_TYPES}\n" + " comfy_low.transport._Prepared.parse_run_result decodes an " + "'application/json' success to a dict and returns anything else as a " + "BinaryResult. A branch added or removed upstream changes what " + "post_model_run returns, so reconcile it rather than widening this list.", + file=sys.stderr, + ) + failed = True if declared_path != _MODEL_RUN_PATH_TEMPLATE: print( f"ERROR: the bound model-run route has drifted from {ROUTER_SPEC.name}.\n" @@ -258,7 +301,10 @@ def _check_router_run_route() -> int: failed = True if failed: return 1 - print(f"OK: the SDK posts a model run to {declared_host}{declared_path}, as the spec declares") + print( + f"OK: the SDK posts a model run to {declared_host}{declared_path} and branches its " + f"200 on {declared_media}, as the spec declares" + ) return 0 diff --git a/src/comfy_low/__init__.py b/src/comfy_low/__init__.py index 75d65d8..ee69413 100644 --- a/src/comfy_low/__init__.py +++ b/src/comfy_low/__init__.py @@ -31,7 +31,7 @@ error_from_envelope, ) from .sse import RawEvent, SSEDecoder -from .transport import AsyncComfyLow, ComfyLow +from .transport import AsyncComfyLow, BinaryResult, ComfyLow # The exact set of operationIds the transport must cover; the spec-coverage test # asserts this equals the set of operationIds in spec/openapi.yaml. @@ -70,6 +70,7 @@ "models", "ComfyLow", "AsyncComfyLow", + "BinaryResult", "RawEvent", "SSEDecoder", "ApiError", diff --git a/src/comfy_low/transport.py b/src/comfy_low/transport.py index c975d41..818d3dc 100644 --- a/src/comfy_low/transport.py +++ b/src/comfy_low/transport.py @@ -24,6 +24,11 @@ — and ``tests/test_router_spec_contract.py`` plus ``scripts/check_drift.py`` fail if that constant and the vendored path disagree. +It is also the one binding whose success may not be JSON: that route's ``200`` +declares a ``*/*`` ``format: binary`` branch beside its ``application/json`` +one, so it returns ``dict | BinaryResult`` and parses through +:meth:`_Prepared.parse_run_result` rather than :meth:`_Prepared.parse_or_raise`. + The four ``post_model_submit`` / ``get_model_request_status`` / ``get_model_request_result`` / ``put_model_request_cancel`` bindings are the same story one step earlier: they are the *queued* form of that one operation, @@ -45,10 +50,11 @@ import sys from collections.abc import AsyncIterator, Iterator, Mapping from contextlib import asynccontextmanager, contextmanager +from dataclasses import dataclass from datetime import datetime, timedelta, timezone from importlib.metadata import PackageNotFoundError from importlib.metadata import version as _pkg_version -from typing import Any, BinaryIO +from typing import Any, BinaryIO, NoReturn, cast from urllib.parse import parse_qs, quote, urlencode, urlsplit, urlunsplit import httpx @@ -126,6 +132,105 @@ _DEFAULT_PORTS = {"http": 80, "https": 443} +#: Longest ``Content-Type`` kept on a :class:`BinaryResult`. Generous next to +#: any real media type and its parameters (``audio/L16; rate=16000; channels=1`` +#: is 34 characters), short enough that a partner-controlled header cannot +#: flood the log line or REPL echo that prints it. +_CONTENT_TYPE_LIMIT = 128 + + +@dataclass(frozen=True) +class BinaryResult: + """A completed model run whose native output is bytes rather than JSON. + + What ``post_model_run`` and ``get_model_request_result`` — and so + ``comfy_sdk.models.run`` and the queued surface's own result fetch — return + instead of a ``dict`` when Comfy Router answers under a media type that is + not JSON. Router forwards the partner model's output *unchanged*, and for a + model whose partner answers a generation directly as bytes (the ElevenLabs + audio models are the first in the catalog) that output is raw audio under + the partner's own ``Content-Type``. Both routes' ``200`` declare the same + two branches — ``application/json`` and a ``*/*`` ``format: binary`` one — + and the spec tells clients to branch on the response ``Content-Type``. + + The bytes are handed over exactly as they arrived: not base64-encoded, not + wrapped in a dict, not decoded or transcoded. The point of the surface is + that the partner's native output comes back unchanged, so writing + ``result.content`` to a file gives you the file the partner produced. + + Frozen because it is a value, not a handle: two runs that produced the same + bytes under the same media type are the same result. + """ + + #: The response body verbatim — the partner's own file bytes. + content: bytes + #: The response's ``Content-Type`` as sent, parameters included + #: (``audio/mpeg``, ``audio/L16; rate=16000``, ...), because for some + #: partner media types the parameters are part of what the bytes are. + #: Bounded and stripped of unprintable characters, which no real media type + #: has. Empty string when the response carried no ``Content-Type`` at all. + content_type: str + #: The server-minted ``X-Comfy-Request-Id`` for the call, or ``None`` when + #: the response named none. The id to quote in a support request — the same + #: one a failure of this call would have carried on ``exc.request_id``. + #: + #: ``None`` is also the one signal that distinguishes a partner's native + #: output from an intermediary's error page. Router's contract marks this + #: header ``required`` on every answer it sends, so a 200 that omits it did + #: not come from Router — it is the ``text/html`` interstitial or the + #: ``no healthy upstream`` text a proxy served in its place. The SDK hands + #: those back rather than raising (see :meth:`_Prepared.parse_run_result`), + #: so a caller who would rather not write one to disk should check this + #: before trusting ``content``. + request_id: str | None + + def __repr__(self) -> str: + # Written out rather than inherited: the dataclass default would render + # the whole body, and this object lands in tracebacks, REPL echoes and + # CI logs holding a multi-megabyte audio file. The length is the part + # anyone reading a repr actually wants. + return ( + f"{type(self).__name__}(content=<{len(self.content)} bytes>, " + f"content_type={self.content_type!r}, request_id={self.request_id!r})" + ) + + +def media_type(content_type: str | None) -> str: + """The bare, lowercased media type of a ``Content-Type`` header value. + + ``"application/json; charset=utf-8"`` -> ``"application/json"``. Returns + ``""`` for ``None`` or for a header that names no type at all, which is the + "the response said nothing" case callers branch on separately from "the + response said something that is not JSON". + + A comma ends the type as surely as a semicolon does. ``httpx.Headers.get`` + joins a header sent *twice* with ``", "`` — the same joining + :func:`comfy_low.errors.clean_request_id` already has to allow for — and + intermediaries do duplicate ``Content-Type``, so a perfectly ordinary JSON + answer can arrive as ``"application/json, application/json"``. Splitting on + the semicolon alone would read that as a type of its own, decide it is not + JSON, and hand a decodable result back as opaque bytes. + """ + if not content_type: + return "" + head = content_type.split(";", 1)[0] + return head.split(",", 1)[0].strip().lower() + + +def is_json_media_type(media: str) -> bool: + """Whether ``media`` (a bare media type) denotes a JSON document. + + ``application/json`` and the ``+json`` structured suffix (RFC 6839), which + is what the run route's ``application/json`` branch covers and what a + partner returning a JSON-shaped result arrives under. Everything else is + the ``*/*`` binary branch as far as this SDK is concerned — deliberately + including ``text/plain`` and ``text/html``: the SDK cannot tell a partner's + native text output from a proxy interstitial, and on this route the + contract says the body is the partner's, so it hands the bytes back rather + than throwing away a generation the caller was billed for. + """ + return media == "application/json" or media.endswith("+json") + def parse_model_id(model: str) -> tuple[str, str]: """Split a canonical ``{provider}/{model}`` id into its two path segments. @@ -498,28 +603,133 @@ def headers(self, url: str, extra: dict[str, str] | None = None) -> dict[str, st def parse_or_raise(self, resp: httpx.Response, ok: tuple[int, ...]) -> dict[str, Any]: if resp.status_code in ok: + return self._decode_json(resp) + self._raise_for_response(resp) + + def parse_run_result( + self, resp: httpx.Response, ok: tuple[int, ...] + ) -> dict[str, Any] | BinaryResult: + """:meth:`parse_or_raise` for an operation whose success may not be JSON. + + Used by ``post_model_run`` and ``get_model_request_result`` — the + awaited and the queued routes for collecting a model run's result — + and nothing else. Comfy Router forwards the partner model's native + output **under the partner's own media type** on both: each route's + ``200`` declares an ``application/json`` branch *and* a ``*/*`` + ``format: binary`` branch, and the spec says in as many words that a + client MUST branch on the response ``Content-Type`` rather than assume + a JSON document. So this is the one place that does, and every other + operation keeps :meth:`parse_or_raise` — including its reading of an + undecodable success as an interstitial, which stays correct for a + route whose only declared success media type is JSON. + + The branch is on the declared type, not on whether the bytes happen to + parse: an ``audio/mpeg`` body that coincidentally started with ``{`` + would still be audio, and JSON that arrived under ``application/json`` + but will not parse is still the truncated/interstitial failure the + caller needs raised rather than handed back as opaque bytes. + + **A non-JSON 2xx is handed back even when it looks like an error page, + and even when it is empty.** A ``text/html`` interstitial and a + zero-length ``audio/mpeg`` body both reach the caller as a + :class:`BinaryResult` rather than raising. That is deliberate and it is + the asymmetry that decides it: this route's 200 means a generation ran + and was billed, so raising on a body the SDK merely finds suspicious + destroys something the caller paid for and cannot get back, while + returning an inspectable object costs them a check. The check is cheap + and it is exact — ``request_id is None`` means no Router answer was + seen at all (the header is ``required`` on every one it sends), and + ``not content`` means nothing was delivered. Gating the branch on + either instead would make this SDK discard a real generation whenever + an intermediary stripped a header or a partner served an empty file, + which is the failure the whole surface exists to stop. + + A success carrying no ``Content-Type`` at all is the one case with + nothing to branch on. An empty body stays ``{}`` (what every other + operation does with one) and a body that parses as a JSON **object** + stays a dict; anything else non-empty becomes a :class:`BinaryResult`, + with ``content_type=""`` to say the response never named one. + + "Object", not merely "valid JSON", because this branch is a *probe* of + arbitrary bytes rather than a decode of a document the response + promised. ``null``, ``[...]`` and a bare number are all valid JSON and + none of them is the run result this method is declared to return, so + accepting one would hand back a ``None``/``list``/``int`` from a + ``dict | BinaryResult`` signature and break the caller who narrowed with + ``isinstance(result, BinaryResult)``. It would also re-open the very + failure this method exists to fix: a short binary body that happens to + be all ASCII digits parses as an ``int``, and the generation's bytes are + gone. Only a dict is a result; everything else is bytes. + """ + if resp.status_code in ok: + media = media_type(resp.headers.get("Content-Type")) + if media: + if is_json_media_type(media): + return self._decode_json(resp) + return self._binary_result(resp) if not resp.content: return {} try: - return resp.json() - except ValueError as exc: - # A success status whose body will not decode — a proxy - # interstitial served as 200, a response truncated mid-stream. - # Raised as an ApiError rather than escaping as the raw - # `json.JSONDecodeError` so it lands on the surface the SDK - # translates and stamps: on `models.run` this is a generation - # that ran and was billed with the result lost, which is - # exactly the failure the Idempotency-Key has to ride out on. - raise ApiError( - f"Could not decode the {resp.status_code} response body as JSON", - code="invalid_response", - http_status=resp.status_code, - request_id=_request_id(resp), - # Whatever was served instead is the only description of - # what answered — the interstitial's own text names the - # proxy, and it is discarded with the response otherwise. - body_excerpt=_body_excerpt(resp), - ) from exc + probed = resp.json() + except (ValueError, RecursionError): + # `RecursionError` beside `ValueError` because this probes bytes + # that were never claimed to be JSON: a long run of `[` is + # syntactically valid and nests until the decoder blows the + # stack, which is not a `ValueError` and would escape as a raw + # exception instead of falling through to the bytes. + return self._binary_result(resp) + if isinstance(probed, dict): + return cast("dict[str, Any]", probed) + return self._binary_result(resp) + self._raise_for_response(resp) + + def _binary_result(self, resp: httpx.Response) -> BinaryResult: + # The whole header rather than the bare media type: the partner's + # parameters are part of what the bytes are (`audio/L16; rate=16000` + # says nothing without its `rate`), and the whole point of the surface + # is that the native output comes back unchanged. + # + # Filtered the way every other server-supplied string this SDK surfaces + # is — `request_id` through `clean_request_id`, body text through + # `clean_body_excerpt` — because the README tells callers to print this + # one, and a partner-controlled header is unbounded and can carry the + # C1/ESC bytes that repaint the terminal reading it. For every media + # type this route actually serves the filter is a no-op, so the value + # stays verbatim exactly where "verbatim" means anything. + return BinaryResult( + content=resp.content, + content_type=clean_body_excerpt( + resp.headers.get("Content-Type"), limit=_CONTENT_TYPE_LIMIT + ) + or "", + request_id=_request_id(resp), + ) + + def _decode_json(self, resp: httpx.Response) -> dict[str, Any]: + if not resp.content: + return {} + try: + return cast("dict[str, Any]", resp.json()) + except ValueError as exc: + # A success status whose body will not decode — a proxy + # interstitial served as 200, a response truncated mid-stream. + # Raised as an ApiError rather than escaping as the raw + # `json.JSONDecodeError` so it lands on the surface the SDK + # translates and stamps: on `models.run` this is a generation + # that ran and was billed with the result lost, which is + # exactly the failure the Idempotency-Key has to ride out on. + raise ApiError( + f"Could not decode the {resp.status_code} response body as JSON", + code="invalid_response", + http_status=resp.status_code, + request_id=_request_id(resp), + # Whatever was served instead is the only description of + # what answered — the interstitial's own text names the + # proxy, and it is discarded with the response otherwise. + body_excerpt=_body_excerpt(resp), + ) from exc + + def _raise_for_response(self, resp: httpx.Response) -> NoReturn: body: dict[str, Any] | None try: body = resp.json() @@ -961,7 +1171,7 @@ def post_model_run( strict_mode: bool | None = None, fallback_provider: bool | str | None = None, timeout: Any = MODEL_RUN_TIMEOUT, - ) -> tuple[dict[str, Any], Mapping[str, str]]: + ) -> tuple[dict[str, Any] | BinaryResult, Mapping[str, str]]: """POST ``{router_base_url}/v2/models/{provider}/{model}`` — awaited server-side. Addressed to Comfy Router, not to the ``/api/v2`` deployment @@ -977,11 +1187,24 @@ def post_model_run( ``arguments`` is sent as the body verbatim (the partner model's native JSON input) and the response body is returned verbatim (its native - output), with no model class layered over either. This is not an + output), with no model class layered over either — a non-JSON output + reaches the caller as the :class:`BinaryResult` carrier described + below, which holds the bytes rather than modelling them. This is not an ``operationId`` of ``spec/openapi.yaml``; it is ``runRouterModel`` of ``spec/router-openapi.yaml``, hand-bound — see :data:`_MODEL_RUN_PATH_TEMPLATE`. + **"Verbatim" includes not being JSON.** Router forwards the partner's + output under the partner's own media type, so the return type is + ``dict`` *or* :class:`BinaryResult`, decided by the response's + ``Content-Type``: ``application/json`` (or a ``+json`` suffix type) + decodes to a dict as before, anything else comes back as a + ``BinaryResult`` holding the bytes unchanged. That is the route's + published ``200``, which declares an ``application/json`` branch and a + ``*/*`` ``format: binary`` one; the models whose partner answers a + generation directly as bytes are the ElevenLabs audio models. See + :meth:`_Prepared.parse_run_result` for the no-``Content-Type`` case. + Returns ``(body, headers)`` rather than the bare body, matching the four ``*_model_request*`` queue methods beside it. The response headers are not incidental on this route: ``X-Comfy-Router-Fallback-Provider`` is the @@ -1004,7 +1227,7 @@ def post_model_run( ) url = self._p.router_base_url + path resp = self.raw_request("POST", url, headers=headers, json=body, timeout=timeout) - return self._p.parse_or_raise(resp, (200, 201)), resp.headers + return self._p.parse_run_result(resp, (200, 201)), resp.headers # -- models: the queued form ------------------------------------------ # @@ -1069,18 +1292,23 @@ def get_model_request_status( def get_model_request_result( self, model: str, request_id: str, *, timeout: Any = _UNSET - ) -> tuple[dict[str, Any], httpx.Headers]: + ) -> tuple[dict[str, Any] | BinaryResult, httpx.Headers]: """GET the finished result of one submitted request. The body is the provider's own payload, exactly as :meth:`post_model_run` returns it — this route is where a queued - request's result is collected, not a differently-shaped one. + request's result is collected, not a differently-shaped one. That + includes the same ``dict`` / :class:`BinaryResult` branch: this route's + published ``200`` declares the identical ``application/json`` and + ``*/*`` ``format: binary`` pair, so a model whose partner answers a + generation directly as bytes is not a JSON document here either. See + :meth:`_Prepared.parse_run_result`. """ url = self._p.router_base_url + model_request_path( model, request_id, _MODEL_REQUEST_PATH_TEMPLATE ) resp = self.raw_request("GET", url, timeout=timeout) - return self._p.parse_or_raise(resp, (200,)), resp.headers + return self._p.parse_run_result(resp, (200,)), resp.headers def put_model_request_cancel( self, model: str, request_id: str, *, timeout: Any = _UNSET @@ -1409,8 +1637,8 @@ async def post_model_run( strict_mode: bool | None = None, fallback_provider: bool | str | None = None, timeout: Any = MODEL_RUN_TIMEOUT, - ) -> tuple[dict[str, Any], Mapping[str, str]]: - """Async :meth:`ComfyLow.post_model_run`.""" + ) -> tuple[dict[str, Any] | BinaryResult, Mapping[str, str]]: + """Async :meth:`ComfyLow.post_model_run` — same ``dict | BinaryResult`` body.""" path, body, headers = model_run_request( model, arguments, @@ -1421,7 +1649,7 @@ async def post_model_run( ) url = self._p.router_base_url + path resp = await self.raw_request("POST", url, headers=headers, json=body, timeout=timeout) - return self._p.parse_or_raise(resp, (200, 201)), resp.headers + return self._p.parse_run_result(resp, (200, 201)), resp.headers # -- models: the queued form ------------------------------------------ async def post_model_submit( @@ -1450,13 +1678,13 @@ async def get_model_request_status( async def get_model_request_result( self, model: str, request_id: str, *, timeout: Any = _UNSET - ) -> tuple[dict[str, Any], httpx.Headers]: + ) -> tuple[dict[str, Any] | BinaryResult, httpx.Headers]: """Async :meth:`ComfyLow.get_model_request_result`.""" url = self._p.router_base_url + model_request_path( model, request_id, _MODEL_REQUEST_PATH_TEMPLATE ) resp = await self.raw_request("GET", url, timeout=timeout) - return self._p.parse_or_raise(resp, (200,)), resp.headers + return self._p.parse_run_result(resp, (200,)), resp.headers async def put_model_request_cancel( self, model: str, request_id: str, *, timeout: Any = _UNSET @@ -1473,4 +1701,4 @@ def _looks_like_path(s: str) -> bool: return s.startswith("http") or s.startswith("/") -__all__ = ["ComfyLow", "AsyncComfyLow", "ApiError"] +__all__ = ["ComfyLow", "AsyncComfyLow", "ApiError", "BinaryResult"] diff --git a/src/comfy_sdk/__init__.py b/src/comfy_sdk/__init__.py index 4a92c09..39bb3e5 100644 --- a/src/comfy_sdk/__init__.py +++ b/src/comfy_sdk/__init__.py @@ -70,6 +70,7 @@ ) from .jobs import AsyncJob, Job, JobWorkflow from .model_requests import COMPLETED, AsyncRequestHandle, QueueUpdate, RequestHandle +from .models import BinaryResult from .outputs import AsyncOutput, DownloadUrl, Output from .retry import DEFAULT_RETRY, NO_RETRY, RetryPolicy from .workflows import Workflow, WorkflowFactory @@ -113,6 +114,7 @@ "Output", "AsyncOutput", "DownloadUrl", + "BinaryResult", # events "Event", "Progress", diff --git a/src/comfy_sdk/model_requests.py b/src/comfy_sdk/model_requests.py index 8913565..5b91b73 100644 --- a/src/comfy_sdk/model_requests.py +++ b/src/comfy_sdk/model_requests.py @@ -47,7 +47,7 @@ import httpx from comfy_low.errors import ApiError -from comfy_low.transport import AsyncComfyLow, ComfyLow, parse_request_id +from comfy_low.transport import AsyncComfyLow, BinaryResult, ComfyLow, parse_request_id from . import _core from .exceptions import ComfyError, translating @@ -489,18 +489,19 @@ def iter_events(self, timeout: float | None = None) -> Generator[QueueUpdate, No delay = _pace(update, backoff) time.sleep(delay if remaining is None else min(delay, remaining)) - def get(self, timeout: float | None = None) -> dict[str, Any]: + def get(self, timeout: float | None = None) -> dict[str, Any] | BinaryResult: """Wait for the request to complete and return the provider's payload. - The result is the partner model's own output, decoded from JSON and - handed back as-is — the same value ``models.run`` returns for the same - model and arguments, under the same ``dict[str, Any]`` annotation. That - annotation is the contract: every model Router serves answers with a - JSON object. A partner whose native output were an array or a bare - value would still be handed back unchanged rather than rejected, since - the payload is the partner's and not this SDK's to reshape — but that - is robustness against an off-contract payload, not a second supported - return type. + The result is a ``dict`` for a model whose partner answers JSON and a + :class:`~comfy_sdk.BinaryResult` for one whose partner answers a + generation directly as bytes — the same ``dict | BinaryResult`` branch + ``models.run`` returns for the same model and arguments, since this + route's ``200`` declares the identical ``application/json`` / ``*/*`` + pair. A partner's JSON output that is an array or a bare value rather + than an object is still handed back unchanged rather than rejected, + since the payload is the partner's and not this SDK's to reshape — but + that is robustness against an off-contract payload, not a third + supported return type. Raises the typed router exception (:mod:`comfy_sdk.router_exceptions`) when the completion carries an @@ -517,7 +518,9 @@ def get(self, timeout: float | None = None) -> dict[str, Any]: completion = _last(self.iter_events(timeout=timeout)) return self._collect(completion, budget=_remaining(deadline)) - def _collect(self, completion: QueueUpdate, *, budget: float | None = None) -> dict[str, Any]: + def _collect( + self, completion: QueueUpdate, *, budget: float | None = None + ) -> dict[str, Any] | BinaryResult: """Turn an observed completion into a result, or into the typed error. Split out of :meth:`get` so ``models.subscribe`` — which has already @@ -536,7 +539,8 @@ def _collect(self, completion: QueueUpdate, *, budget: float | None = None) -> d ) # Checked again on the result body: which of the two responses carries # the `error_type` is the server's choice, and reading only one of them - # is how a failure gets returned as a result. + # is how a failure gets returned as a result. A `BinaryResult` is not a + # `Mapping` and so never carries one -- it is bytes, not an envelope. _raise_for_completion(payload, request_id=self._request_id, envelope_only=True) return payload @@ -576,11 +580,11 @@ def _cancel_best_effort(self) -> None: def _call( self, - send: Callable[[], tuple[dict[str, Any], httpx.Headers]], + send: Callable[[], tuple[dict[str, Any] | BinaryResult, httpx.Headers]], *, budget: float | None = None, policy: RetryPolicy | None = None, - ) -> tuple[dict[str, Any], httpx.Headers]: + ) -> tuple[dict[str, Any] | BinaryResult, httpx.Headers]: """Run one queue call under the client's retry policy. The same ``Retrier`` ``models.run`` uses, constructed per call because @@ -654,7 +658,7 @@ async def iter_events(self, timeout: float | None = None) -> AsyncGenerator[Queu delay = _pace(update, backoff) await asyncio.sleep(delay if remaining is None else min(delay, remaining)) - async def get(self, timeout: float | None = None) -> dict[str, Any]: + async def get(self, timeout: float | None = None) -> dict[str, Any] | BinaryResult: """Async :meth:`RequestHandle.get` — wait, then collect or raise.""" deadline = None if timeout is None else _now() + timeout completion: QueueUpdate | None = None @@ -664,7 +668,7 @@ async def get(self, timeout: float | None = None) -> dict[str, Any]: async def _collect( self, completion: QueueUpdate, *, budget: float | None = None - ) -> dict[str, Any]: + ) -> dict[str, Any] | BinaryResult: """Async :meth:`RequestHandle._collect`.""" _raise_for_completion(completion.raw, request_id=self._request_id) with translating(): @@ -697,11 +701,11 @@ async def _cancel_best_effort(self) -> None: async def _call( self, - send: Callable[[], Awaitable[tuple[dict[str, Any], httpx.Headers]]], + send: Callable[[], Awaitable[tuple[dict[str, Any] | BinaryResult, httpx.Headers]]], *, budget: float | None = None, policy: RetryPolicy | None = None, - ) -> tuple[dict[str, Any], httpx.Headers]: + ) -> tuple[dict[str, Any] | BinaryResult, httpx.Headers]: """Async :meth:`RequestHandle._call` — one queue call under the retry policy.""" retrier = Retrier(_bounded(policy or self._retry, budget), now=_now) while True: diff --git a/src/comfy_sdk/models.py b/src/comfy_sdk/models.py index 61eaaa8..c1afe42 100644 --- a/src/comfy_sdk/models.py +++ b/src/comfy_sdk/models.py @@ -35,7 +35,10 @@ to convention. Callers do not import anything for this: ``from comfy_sdk import Comfy`` stays -the only entry point, and ``client.models`` is the whole surface. +the only entry point, and ``client.models`` is the whole surface. The one name +worth importing is :class:`~comfy_low.transport.BinaryResult`, re-exported here +and from ``comfy_sdk``, for an ``isinstance`` check on a run whose model answers +in bytes rather than JSON. """ from __future__ import annotations @@ -57,6 +60,7 @@ from comfy_low.transport import ( MODEL_RUN_TIMEOUT, AsyncComfyLow, + BinaryResult, ComfyLow, parse_model_id, parse_request_id, @@ -222,8 +226,15 @@ class RouterRunResult: is the only place it is disclosed at all. """ - output: dict[str, Any] - """The partner model's native JSON output, exactly what :meth:`Models.run` returns.""" + output: dict[str, Any] | BinaryResult + """The partner model's native output, exactly what :meth:`Models.run` returns. + + A ``dict`` for a model whose partner answers JSON, and a + :class:`~comfy_sdk.BinaryResult` for one whose partner answers a generation + directly as bytes — the same two shapes, decided the same way, as + :meth:`Models.run`. ``run_detailed`` adds the Router disclosures beside the + output; it does not change what the output is. + """ serving_provider: str | None """``X-Comfy-Router-Fallback-Provider``: the provider that ultimately served this call. @@ -362,7 +373,7 @@ def _credits_used(raw: str | None) -> str | None: return candidate if parsed.is_finite() else None -def _run_result(body: dict[str, Any], headers: Mapping[str, str]) -> RouterRunResult: +def _run_result(body: dict[str, Any] | BinaryResult, headers: Mapping[str, str]) -> RouterRunResult: """Build a :class:`RouterRunResult` from one run's body and response headers.""" return RouterRunResult( output=body, @@ -395,7 +406,7 @@ def _run( strict_mode: bool | None = None, fallback_provider: bool | str | None = None, timeout: float | httpx.Timeout | None = MODEL_RUN_TIMEOUT, - ) -> tuple[dict[str, Any], Mapping[str, str]]: + ) -> tuple[dict[str, Any] | BinaryResult, Mapping[str, str]]: """Run ``model`` with ``arguments`` and return the completed result. ``model`` is the canonical ``{provider}/{model}`` id — exactly the two @@ -435,9 +446,27 @@ def _run( awaitable form of this method is :meth:`AsyncModels.run` on ``AsyncComfy``. - The return value is the provider's own payload, decoded from JSON and - handed back as-is — no wrapper class stands between the caller and the - fields the provider documented. + The return value is the provider's own payload, handed back as-is. It + comes in **two shapes**, decided by the response's ``Content-Type``, + because Router forwards the partner's output under the partner's own + media type: + + * a ``dict`` — the provider's JSON document, decoded, with its own field + names untouched. This is what all but a couple of models in the + catalog return, and it is unchanged from previous releases. + * a :class:`~comfy_low.transport.BinaryResult` — for a model whose + partner answers a generation directly as bytes (the ElevenLabs audio + models are the first of these). ``result.content`` is the file bytes + exactly as they arrived, ``result.content_type`` the media type the + response named, ``result.request_id`` its ``X-Comfy-Request-Id``. The + bytes are not base64-encoded and not wrapped in a dict: write them to + a file and you have the file the partner produced. + + Branch with ``isinstance(result, BinaryResult)`` when you call a model + that might do either; a model's own contract (``GET + /v2/models/{provider}/{model}/openapi.json``) says which it is, and a + ``200`` whose ``Content-Type`` claims JSON but whose body will not parse + is still an error rather than bytes. Because the server may legitimately hold the connection for minutes, ``timeout`` defaults to :data:`~comfy_low.transport.MODEL_RUN_TIMEOUT` @@ -628,12 +657,17 @@ def run( strict_mode: bool | None = None, fallback_provider: bool | str | None = None, timeout: float | httpx.Timeout | None = MODEL_RUN_TIMEOUT, - ) -> dict[str, Any]: + ) -> dict[str, Any] | BinaryResult: """Run ``model`` with ``arguments`` and return the partner's native output. See :meth:`_run` for the full contract; this is that call, answering the result document alone. + The result is a ``dict`` for a model whose partner answers JSON and a + :class:`~comfy_sdk.BinaryResult` for one whose partner answers a + generation directly as bytes, branched on the response ``Content-Type`` + exactly as the run route's published ``200`` says a client must. + ``run`` answers the native output because that document is what a caller asked for; ``run_detailed`` answers a :class:`RouterRunResult`, which carries that same output plus what Router disclosed about HOW the call @@ -757,14 +791,16 @@ def subscribe( on_queue_update: Callable[[QueueUpdate], Any] | None = None, timeout: float | None = None, idempotency_key: str | None = None, - ) -> dict[str, Any]: + ) -> dict[str, Any] | BinaryResult: """Queue a request, follow it to completion, and return its result. :meth:`submit` plus polling plus :meth:`~comfy_sdk.model_requests.RequestHandle.get`, in one call — the ergonomic form for a caller who does want to wait but also wants to show progress while waiting. The return value is the provider's own - payload, identical to what :meth:`run` would have returned. + payload, identical to what :meth:`run` would have returned — a ``dict`` + for JSON output, a :class:`BinaryResult` for a model whose partner + answers a generation directly as bytes. ``on_queue_update`` is called with a :class:`~comfy_sdk.model_requests.QueueUpdate` each time the queue @@ -865,7 +901,7 @@ async def _run( strict_mode: bool | None = None, fallback_provider: bool | str | None = None, timeout: float | httpx.Timeout | None = MODEL_RUN_TIMEOUT, - ) -> tuple[dict[str, Any], Mapping[str, str]]: + ) -> tuple[dict[str, Any] | BinaryResult, Mapping[str, str]]: """Awaitable :meth:`Models.run` — same arguments, same result shape. This *is* the async form of ``run``: awaiting it on ``AsyncComfy`` is @@ -930,8 +966,8 @@ async def run( strict_mode: bool | None = None, fallback_provider: bool | str | None = None, timeout: float | httpx.Timeout | None = MODEL_RUN_TIMEOUT, - ) -> dict[str, Any]: - """Awaitable :meth:`Models.run` — same arguments, same result shape.""" + ) -> dict[str, Any] | BinaryResult: + """Awaitable :meth:`Models.run` — same ``dict | BinaryResult`` result.""" body, _ = await self._run( model, arguments, @@ -1009,7 +1045,7 @@ async def subscribe( on_queue_update: Callable[[QueueUpdate], Any] | None = None, timeout: float | None = None, idempotency_key: str | None = None, - ) -> dict[str, Any]: + ) -> dict[str, Any] | BinaryResult: """Awaitable :meth:`Models.subscribe` — same arguments, same result. ``on_queue_update`` may be a plain callable or a coroutine function; @@ -1071,3 +1107,6 @@ async def handle(self, model: str, request_id: str) -> AsyncRequestHandle: parse_model_id(model) parse_request_id(request_id) return AsyncRequestHandle(cast(AsyncComfyLow, self._low), model, request_id, self._retry) + + +__all__ = ["Models", "AsyncModels", "BinaryResult", "RouterRunResult"] diff --git a/tests/conftest.py b/tests/conftest.py index e8fe3c7..c790148 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -122,6 +122,21 @@ class ServerState: # interstitial served under a 200, a response truncated mid-stream. The # generation ran and was billed; only the result is unreadable. model_run_undecodable_body: bool = False + # The Content-Type that undecodable body is served under. `application/json` + # by default, because that is the case that is still an ERROR: the response + # promised a JSON document and did not deliver one. Point it at `text/html` + # and the same body is instead a success carrying non-JSON bytes, which is + # what the run route's `*/*` branch says to do with it — the SDK cannot tell + # a proxy's interstitial from a partner's native text output, and on this + # route the contract says the body is the partner's. + model_run_undecodable_content_type: str = "application/json" + # Answer a successful run with these raw bytes under + # `model_run_binary_content_type` instead of `model_run_result` as JSON — + # the ElevenLabs-shaped direct-return binary 200. `None` serves JSON. + model_run_binary_body: bytes | None = None + # Content-Type for `model_run_binary_body`. `None` sends no Content-Type + # header at all, which is the header-stripping-intermediary case. + model_run_binary_content_type: str | None = "audio/mpeg" # Model the deployment `retry_possibly_in_flight` exists for: one that # *replays* a repeated Idempotency-Key rather than rejecting it, so a key # is released rather than claimed when a request fails 5xx. Default False @@ -151,8 +166,10 @@ class ServerState: # 409). `None` sends no header at all, which is the same failure the policy # must *not* retry. model_run_retry_after: str | None = None - # Sent as X-Comfy-Request-Id alongside a failed run. `None` sends no header, - # which is the response an intermediary that never reached the router gives. + # Sent as X-Comfy-Request-Id on a model run's answer, success or failure — + # Router stamps it on both, and `BinaryResult.request_id` is read off a + # success. `None` sends no header, which is the response an intermediary + # that never reached the router gives. model_run_request_id: str | None = None # Extra response headers stamped on a SUCCESSFUL model run, for the # disclosure headers the body cannot carry (X-Comfy-Credits-Used, @@ -239,6 +256,13 @@ class ServerState: "seed": 7, } ) + # Answer a completed result with these raw bytes under + # `queue_result_binary_content_type` instead of `queue_result` as JSON — + # the queued sibling of `model_run_binary_body`. `None` serves JSON. + queue_result_binary_body: bytes | None = None + # Content-Type for `queue_result_binary_body`. `None` sends no Content-Type + # header at all, which is the header-stripping-intermediary case. + queue_result_binary_content_type: str | None = "audio/mpeg" # Status code for the cancel response; 204 exercises the empty-body path. queue_cancel_status: int = 200 # Cancels that answer a transient failure (status, code) before one is @@ -314,7 +338,16 @@ class ServerState: # Idempotency-Key -> the result recorded for it under # `model_run_replays_lost_result`, served verbatim to a later request # presenting the same key. - model_run_replay_store: dict[str, dict[str, Any]] = field(default_factory=dict) + # + # The whole answer is recorded, not just the JSON payload: `(payload, + # binary_body, binary_content_type)` as they stood when the generation + # completed. A replay has to serve *that record* rather than re-read the + # knobs, or a test asserting the recorded result came back would pass even + # with the per-key record wrong or empty -- on the one path where serving + # the wrong record means double-billing. + model_run_replay_store: dict[str, tuple[dict[str, Any], bytes | None, str | None]] = field( + default_factory=dict + ) # How many times the model actually *ran*, as distinct from how many # requests arrived (`model_run_count`). A replay serves a recorded result # and does not increment this, which is what lets a test tell a real replay @@ -424,12 +457,25 @@ def _json(self, status: int, payload: dict, headers: dict | None = None) -> None self.end_headers() self.wfile.write(body) - def _raw(self, status: int, body: bytes, content_type: str) -> None: + def _raw( + self, + status: int, + body: bytes, + content_type: str | None, + headers: dict | None = None, + ) -> None: """A response whose body is *not* JSON — the case a client that - calls ``.json()`` unguarded on a success status falls over on.""" + calls ``.json()`` unguarded on a success status falls over on. + + ``content_type=None`` sends **no** ``Content-Type`` header at all, + which is a real shape (an intermediary that strips it) and the one a + client branching on the header has nothing to branch on.""" self.send_response(status) - self.send_header("Content-Type", content_type) + if content_type is not None: + self.send_header("Content-Type", content_type) self.send_header("Content-Length", str(len(body))) + for k, v in (headers or {}).items(): + self.send_header(k, v) self.end_headers() self.wfile.write(body) @@ -750,6 +796,11 @@ def _serve_queue_status(self, request_id: str) -> None: def _serve_queue_result(self, request_id: str) -> None: state.queue_result_count += 1 state.queue_paths.append(self.path) + if state.queue_result_binary_body is not None: + self._raw( + 200, state.queue_result_binary_body, state.queue_result_binary_content_type + ) + return if state.queue_result_raw is not None: self._json(200, state.queue_result_raw) return @@ -822,18 +873,20 @@ def _post_model_run(self, provider: str, model: str) -> None: # rather than rejecting the resend, and the model does not run # again — which is the whole point of asking under the same key. if key and key in state.model_run_replay_store: + recorded_payload, recorded_body, recorded_type = state.model_run_replay_store[key] # `model_run_response_headers` is merged in here as well as on # the fresh-run path below, because a replay is the canonical # reported-zero and the only response where `credits_used` and # `replayed` are both meaningful at once. Stamped first, so the # replay marker itself cannot be overwritten by a test's dict. - self._json( + self._serve_run_result( 200, - state.model_run_replay_store[key], + recorded_payload, headers={ **state.model_run_response_headers, "Idempotent-Replayed": "true", }, + binary=(recorded_body, recorded_type), ) return @@ -894,7 +947,11 @@ def fail(status: int, code: str, message: str) -> None: # The generation completed; only the answer was lost. Bill # it once and record it, so the same key collects it. state.model_run_generations += 1 - state.model_run_replay_store[key] = state.model_run_result + state.model_run_replay_store[key] = ( + state.model_run_result, + state.model_run_binary_body, + state.model_run_binary_content_type, + ) headers: dict[str, str] = {} if state.model_run_retry_after is not None: headers["Retry-After"] = state.model_run_retry_after @@ -944,15 +1001,48 @@ def fail(status: int, code: str, message: str) -> None: self._raw( state.model_run_status, b"502 from an intermediary", - "text/html", + state.model_run_undecodable_content_type, ) return - self._json( + self._serve_run_result( state.model_run_status, state.model_run_result, headers=state.model_run_response_headers or None, ) + def _serve_run_result( + self, + status: int, + payload: dict, + headers: dict | None = None, + binary: tuple[bytes | None, str | None] | None = None, + ) -> None: + """A successful run's body — the partner's JSON, or its own bytes. + + Both shapes go through one helper so the *replay* of a claimed key + answers in whichever shape the run itself would have: the route's + ``Idempotent-Replayed`` 200 carries the recorded result, and a + recorded result that was audio is still audio. + + ``binary`` is that record's own ``(body, content_type)``, passed by + the replay branch so the replay serves what was stored against the + key instead of whatever the knobs say *now*. A fresh run passes + none and reads the knobs, which for it are the same thing. + """ + body, content_type = ( + binary + if binary is not None + else (state.model_run_binary_body, state.model_run_binary_content_type) + ) + if state.model_run_request_id is not None: + # Router stamps the id on every answer, not only on failures; + # `BinaryResult.request_id` is read off a *success*. + headers = {**(headers or {}), "X-Comfy-Request-Id": state.model_run_request_id} + if body is not None: + self._raw(status, body, content_type, headers=headers) + return + self._json(status, payload, headers=headers) + def _post_jobs(self) -> None: state.submit_count += 1 body = json.loads(self._read_body() or b"{}") diff --git a/tests/test_models_queue.py b/tests/test_models_queue.py index 04720e1..1bc6f07 100644 --- a/tests/test_models_queue.py +++ b/tests/test_models_queue.py @@ -33,7 +33,7 @@ _MODEL_REQUESTS_PATH_TEMPLATE, _MODEL_RUN_PATH_TEMPLATE, ) -from comfy_sdk import AsyncComfy, Comfy, QueueUpdate +from comfy_sdk import AsyncComfy, BinaryResult, Comfy, QueueUpdate from comfy_sdk.exceptions import ComfyError from comfy_sdk.model_requests import COMPLETED, AsyncRequestHandle, RequestHandle from comfy_sdk.retry import NO_RETRY @@ -188,6 +188,37 @@ def test_get_polls_to_completion_then_collects_the_result(server, fast_poll) -> assert server.state.queue_result_count == 1 +def test_get_returns_a_binary_result_for_a_non_json_completion(server, fast_poll) -> None: + # The queued sibling of `test_an_audio_200_returns_the_bytes_verbatim` in + # `tests/test_models_run_binary.py`: `getRouterModelRequestResult`'s 200 + # declares the identical `application/json` / `*/*` branch `runRouterModel` + # does, so a model whose partner answers a generation directly as bytes is + # not a JSON document on the queued result route either. Before this, + # `get_model_request_result` still went through `parse_or_raise`, which + # calls `.json()` unconditionally and raised `invalid_response` on exactly + # this response -- collecting through the queue, unlike `run`, threw the + # generation away. + server.state.queue_result_binary_body = b"\xff\xfb\x90\x64" + b"\xde\xad" * 64 + server.state.queue_result_binary_content_type = "audio/mpeg" + with _client() as client: + result = client.models.submit(MODEL, ARGS).get() + + assert isinstance(result, BinaryResult) + assert result.content == server.state.queue_result_binary_body + assert result.content_type == "audio/mpeg" + + +def test_subscribe_returns_a_binary_result_too(server, fast_poll) -> None: + server.state.queue_result_binary_body = b"binary generation" + server.state.queue_result_binary_content_type = "audio/wav" + with _client() as client: + result = client.models.subscribe(MODEL, ARGS) + + assert isinstance(result, BinaryResult) + assert result.content == b"binary generation" + assert result.content_type == "audio/wav" + + def test_iter_events_yields_the_first_state_every_change_and_the_completion( server, fast_poll ) -> None: @@ -633,6 +664,18 @@ async def test_async_submit_and_get(server, fast_poll) -> None: assert await handle.get() == server.state.queue_result +async def test_async_get_returns_a_binary_result_too(server, fast_poll) -> None: + server.state.queue_result_binary_body = b"\xff\xfb\x90\x64" + b"\xde\xad" * 64 + server.state.queue_result_binary_content_type = "audio/mpeg" + async with AsyncComfy(api_key="comfyui-test-key") as client: + handle = await client.models.submit(MODEL, ARGS) + result = await handle.get() + + assert isinstance(result, BinaryResult) + assert result.content == server.state.queue_result_binary_body + assert result.content_type == "audio/mpeg" + + async def test_async_iter_events(server, fast_poll) -> None: server.state.queue_polls_to_complete = 2 async with AsyncComfy(api_key="comfyui-test-key") as client: diff --git a/tests/test_models_run.py b/tests/test_models_run.py index 54a9974..5023953 100644 --- a/tests/test_models_run.py +++ b/tests/test_models_run.py @@ -1087,7 +1087,7 @@ def test_run_detailed_reports_a_replay_off_a_real_replayed_response(server) -> N """ with Comfy(retry=NO_RETRY) as client: key = "replay-name-pin" - server.state.model_run_replay_store[key] = server.state.model_run_result + server.state.model_run_replay_store[key] = (server.state.model_run_result, None, None) got = client.models.run_detailed(MODEL, ARGS, idempotency_key=key) assert got.replayed is True @@ -1100,7 +1100,7 @@ def test_a_replayed_run_can_report_its_credits_too(server) -> None: server.state.model_run_response_headers = {"X-Comfy-Credits-Used": "0"} with Comfy(retry=NO_RETRY) as client: key = "replay-with-credits" - server.state.model_run_replay_store[key] = server.state.model_run_result + server.state.model_run_replay_store[key] = (server.state.model_run_result, None, None) got = client.models.run_detailed(MODEL, ARGS, idempotency_key=key) assert got.replayed is True # Reported zero, not absent — the distinction the field exists to keep. diff --git a/tests/test_models_run_binary.py b/tests/test_models_run_binary.py new file mode 100644 index 0000000..5487d44 --- /dev/null +++ b/tests/test_models_run_binary.py @@ -0,0 +1,491 @@ +"""``client.models.run`` when the model's native output is bytes, not JSON. + +Comfy Router forwards a partner model's output *unchanged*, under the partner's +own media type. For most of the catalog that is a JSON document; for a model +whose partner answers a generation directly as bytes — the ElevenLabs audio +models are the first of these — it is raw audio under `audio/mpeg` (or whatever +the requested output format is). The run route's published ``200`` declares both +branches, ``application/json`` and a ``*/*`` ``format: binary`` one, and says in +as many words that a client MUST branch on the response ``Content-Type``. + +Before this, the SDK called ``.json()`` on every 2xx and raised +``invalid_response`` — so a generation that had already run, and already been +billed, was thrown away by the client that asked for it. That is the failure +these tests pin closed, from both clients. + +Two boundaries are as important as the happy path and are asserted here: + +* the JSON branch is untouched — a 200 whose ``Content-Type`` says JSON and + whose body will not parse is still the interstitial/truncation error it was, + because on *that* branch the response promised a document and did not deliver + one; and +* the binary branch still runs inside ``translating(...)``, so the + ``Idempotency-Key`` that is a caller's only handle on an already-billed + generation rides out on a failure and a replayed binary 200 comes back the + same way a first run does. + +Everything here runs against the stubbed server in ``conftest.py``. +""" + +from __future__ import annotations + +import dataclasses + +import pytest + +from comfy_low.transport import is_json_media_type, media_type +from comfy_sdk import NO_RETRY, AsyncComfy, BinaryResult, Comfy +from comfy_sdk.exceptions import ComfyError + +MODEL = "acme/flux-dev" +ARGS = {"prompt": "a cat", "steps": 4} + +#: A body shaped like the one the failure was reported on: an ID3v2 header +#: followed by an MP3 frame sync (`0xff 0xfb`). The `0xff` is what +#: `UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff` was raised on, so +#: a regression here fails exactly the way the original report did. +AUDIO = b"ID3\x04\x00\x00\x00\x00\x00\x00" + b"\x00" * 35 + b"\xff\xfb\x90\x64" + b"\xde\xad" * 64 + + +def _serve_audio(server, content_type: str | None = "audio/mpeg") -> None: + server.state.model_run_binary_body = AUDIO + server.state.model_run_binary_content_type = content_type + + +# --- the reported failure: an audio/mpeg 200 is a result, not an error ---- + + +def test_an_audio_200_returns_the_bytes_verbatim(server) -> None: + _serve_audio(server) + server.state.model_run_request_id = "req_audio_01" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content == AUDIO + assert result.content_type == "audio/mpeg" + assert result.request_id == "req_audio_01" + # The generation was billed once and handed back once — the whole point. + assert server.state.model_run_generations == 1 + + +async def test_the_async_client_returns_the_same_binary_result(server) -> None: + _serve_audio(server) + server.state.model_run_request_id = "req_audio_01" + async with AsyncComfy(retry=NO_RETRY) as client: + result = await client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content == AUDIO + assert result.content_type == "audio/mpeg" + assert result.request_id == "req_audio_01" + + +def test_a_binary_200_raises_nothing(server) -> None: + # Stated on its own because the defect was an *exception*, not a wrong + # value: `pytest.raises` passing elsewhere is not the same assertion. + _serve_audio(server) + with Comfy(retry=NO_RETRY) as client: + client.models.run(MODEL, ARGS) # no exception + + +def test_a_response_with_no_request_id_header_gives_none(server) -> None: + _serve_audio(server) + server.state.model_run_request_id = None + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.request_id is None + + +def test_a_created_shaped_binary_success_is_also_a_result(server) -> None: + # 201 is in the route's ok set alongside 200; the branch is on the media + # type, not on which success status carried it. + _serve_audio(server) + server.state.model_run_status = 201 + with Comfy(retry=NO_RETRY) as client: + assert isinstance(client.models.run(MODEL, ARGS), BinaryResult) + + +# --- the media type is carried verbatim, parameters included -------------- + + +@pytest.mark.parametrize( + "content_type", + [ + "audio/mpeg", + # The format ElevenLabs returns for a `pcm_*` output_format: the + # parameter is not decoration, it is how many samples a second the + # bytes are. Dropping it would leave the caller holding unplayable PCM. + "audio/L16; rate=16000", + "audio/wav", + "application/octet-stream", + "image/png", + "video/mp4", + ], +) +def test_the_content_type_reaches_the_caller_unchanged(server, content_type: str) -> None: + _serve_audio(server, content_type) + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content_type == content_type + assert result.content == AUDIO + + +def test_a_json_content_type_with_a_charset_is_still_the_dict_branch(server) -> None: + # `application/json; charset=utf-8` is JSON. Branching on the raw header + # rather than its media type would have sent it down the binary path. + # + # Served through the binary knobs rather than by leaving them unset: the + # stub's JSON path sends a bare `application/json`, so a test that left + # them alone would never put a parameter on the wire and would pass just as + # happily if `media_type` stopped stripping one. + server.state.model_run_binary_body = b'{"images": [], "seed": 7}' + server.state.model_run_binary_content_type = "application/json; charset=utf-8" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert result == {"images": [], "seed": 7} + + +def test_a_content_type_sent_twice_is_still_the_dict_branch(server) -> None: + # An intermediary that duplicates `Content-Type` reaches httpx as one + # header joined with ", ". Splitting on the semicolon alone read that as a + # media type of its own, found it was not JSON, and handed a perfectly + # decodable result back as opaque bytes -- a silent regression for callers + # who never left the JSON branch. + server.state.model_run_binary_body = b'{"images": [], "seed": 7}' + server.state.model_run_binary_content_type = "application/json, application/json" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert result == {"images": [], "seed": 7} + + +def test_a_json_suffix_media_type_is_the_dict_branch(server) -> None: + # RFC 6839's `+json` structured suffix — a partner answering under its own + # vendor media type is still handing back a JSON document. + server.state.model_run_binary_body = b'{"ok": true}' + server.state.model_run_binary_content_type = "application/vnd.acme.result+json" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert result == {"ok": True} + + +# --- the JSON branch keeps the reading the binary branch takes away ------- + + +def test_a_json_content_type_whose_body_will_not_parse_is_still_an_error(server) -> None: + # The "interstitial served as 200" reading belongs *here* and only here: + # the response promised a JSON document and did not deliver one, so there + # is nothing to hand back. The key still rides out on it. + server.state.model_run_undecodable_body = True + server.state.model_run_undecodable_content_type = "application/json" + with Comfy(retry=NO_RETRY) as client: + with pytest.raises(ComfyError) as excinfo: + client.models.run(MODEL, ARGS) + assert excinfo.value.code == "invalid_response" + assert excinfo.value.http_status == 200 + assert excinfo.value.idempotency_key is not None + + +async def test_the_async_json_branch_raises_the_same_way(server) -> None: + server.state.model_run_undecodable_body = True + server.state.model_run_undecodable_content_type = "application/json" + async with AsyncComfy(retry=NO_RETRY) as client: + with pytest.raises(ComfyError) as excinfo: + await client.models.run(MODEL, ARGS) + assert excinfo.value.code == "invalid_response" + assert excinfo.value.idempotency_key is not None + + +def test_a_text_html_200_now_comes_back_as_bytes(server) -> None: + # The deliberate consequence of branching on the declared type: an HTML + # body under a 200 is no longer read as a proxy interstitial. The SDK + # cannot tell one from a partner's own text output, and on this route the + # contract says the body is the partner's — so throwing away a generation + # the caller was billed for is the worse of the two mistakes. The bytes, + # interstitial or not, reach the caller, who can see what answered. + server.state.model_run_undecodable_body = True + server.state.model_run_undecodable_content_type = "text/html" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content_type == "text/html" + assert b"502 from an intermediary" in result.content + + +# --- a success that names no media type at all --------------------------- + + +def test_no_content_type_and_an_unparseable_body_is_a_binary_result(server) -> None: + # Nothing to branch on, so the body decides — and an empty `content_type` + # says the response named none rather than naming the wrong one. + _serve_audio(server, content_type=None) + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content == AUDIO + assert result.content_type == "" + + +def test_no_content_type_but_a_json_body_is_still_a_dict(server) -> None: + # The other half of that: a header-stripping intermediary in front of a + # JSON model must not turn every result into opaque bytes. + server.state.model_run_binary_body = b'{"images": [], "seed": 7}' + server.state.model_run_binary_content_type = None + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert result == {"images": [], "seed": 7} + + +def test_no_content_type_and_an_empty_body_is_still_an_empty_dict(server) -> None: + # Unchanged from every other operation: an empty success body is `{}`, not + # a `BinaryResult` holding zero bytes. + server.state.model_run_binary_body = b"" + server.state.model_run_binary_content_type = None + with Comfy(retry=NO_RETRY) as client: + assert client.models.run(MODEL, ARGS) == {} + + +@pytest.mark.parametrize( + ("body", "description"), + [ + (b"null", "a JSON null"), + (b"[1, 2, 3]", "a JSON array"), + # The one that re-opens the bug this surface exists to fix: a short + # binary body of all-ASCII digits is valid JSON, and accepting the + # number would drop the generation's bytes on the floor. + (b"1234", "a bare JSON number"), + (b'"a string"', "a bare JSON string"), + (b"[" * 5000, "a nesting depth the decoder cannot walk"), + ], +) +def test_no_content_type_and_a_non_object_body_is_bytes(server, body, description) -> None: + # The headerless branch *probes* arbitrary bytes rather than decoding a + # document the response promised, so only a JSON object counts as a result. + # Returning the `None`/`list`/`int` would hand back something outside the + # declared `dict | BinaryResult` union, and a caller who narrowed with + # `isinstance(result, BinaryResult)` would meet a bare `TypeError` on the + # subscript that follows. The deep-nesting case also pins that the probe + # stays total: `json` raises `RecursionError` there, not `ValueError`. + server.state.model_run_binary_body = body + server.state.model_run_binary_content_type = None + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult), description + assert result.content == body + assert result.content_type == "" + + +def test_a_binary_200_with_an_empty_body_is_an_empty_binary_result(server) -> None: + # Pinned rather than raised, and deliberately. A declared `audio/mpeg` 200 + # means a generation ran and was billed, so refusing to return it destroys + # something the caller paid for; handing back an inspectable object costs + # them `if not result.content`. The check is what the docstring points at, + # so it has to actually hold. + server.state.model_run_binary_body = b"" + server.state.model_run_binary_content_type = "audio/mpeg" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content == b"" + assert result.content_type == "audio/mpeg" + + +def test_a_content_type_carrying_control_bytes_is_filtered(server) -> None: + # The README tells callers to print `content_type`, and this header is + # partner-controlled -- so it is reduced the way every other server-supplied + # string this SDK surfaces is, rather than put on a terminal verbatim. The + # media type itself survives; only what no real one contains is stripped. + server.state.model_run_binary_body = AUDIO + server.state.model_run_binary_content_type = "audio/mpeg\x1b[31m; rate=16000" + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert "\x1b" not in result.content_type + assert result.content_type.startswith("audio/mpeg") + + +def test_a_long_content_type_is_bounded(server) -> None: + server.state.model_run_binary_body = AUDIO + server.state.model_run_binary_content_type = "audio/mpeg; note=" + "x" * 4000 + with Comfy(retry=NO_RETRY) as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert len(result.content_type) <= 128 + + +# --- the key still rides out, and a replay is a result --------------------- + + +def test_a_binary_run_still_sends_and_records_an_idempotency_key(server) -> None: + _serve_audio(server) + with Comfy(retry=NO_RETRY) as client: + client.models.run(MODEL, ARGS) + assert server.state.model_run_idempotency_keys[-1] + + +def test_a_failure_on_the_binary_path_still_carries_the_key(server) -> None: + # The binary branch does not move the call out of `translating(...)`: a + # model that answers in bytes still fails like any other, and the key is + # still the caller's handle on a generation they may already owe for. + _serve_audio(server) + server.state.model_run_error = (503, "internal_error") + with Comfy(retry=NO_RETRY) as client: + with pytest.raises(ComfyError) as excinfo: + client.models.run(MODEL, ARGS) + assert excinfo.value.idempotency_key is not None + assert excinfo.value.idempotency_key == server.state.model_run_idempotency_keys[0] + + +def test_a_replayed_binary_200_is_returned_like_a_first_run(server) -> None: + # The replay contract end to end: the first attempt's response is lost + # (a 5xx after the generation completed), the caller resends the key they + # were handed, and the recorded result comes back under + # `Idempotent-Replayed` — as the same bytes, not as an exception, and + # without the model running a second time. + _serve_audio(server) + server.state.model_run_error = (504, "deadline_exceeded") + server.state.model_run_replays_lost_result = True + with Comfy(retry=NO_RETRY) as client: + with pytest.raises(ComfyError) as excinfo: + client.models.run(MODEL, ARGS) + key = excinfo.value.idempotency_key + assert key is not None + + server.state.model_run_error = None + # Moved out from under the stub before the resend: the replay must + # answer from the record it stored against this key, not from whatever + # the knobs say now. Without that the assertion below would hold even + # with the per-key record wrong or empty -- on the one path where + # serving the wrong record means billing the caller twice. + server.state.model_run_binary_body = b"bytes from a different generation" + server.state.model_run_binary_content_type = "audio/wav" + replayed = client.models.run(MODEL, ARGS, idempotency_key=key) + + assert isinstance(replayed, BinaryResult) + assert replayed.content == AUDIO + assert replayed.content_type == "audio/mpeg" + # Billed once across both requests: the replay served the record. + assert server.state.model_run_count == 2 + assert server.state.model_run_generations == 1 + + +async def test_the_async_replay_of_a_binary_200_behaves_the_same(server) -> None: + _serve_audio(server) + server.state.model_run_error = (504, "deadline_exceeded") + server.state.model_run_replays_lost_result = True + async with AsyncComfy(retry=NO_RETRY) as client: + with pytest.raises(ComfyError) as excinfo: + await client.models.run(MODEL, ARGS) + key = excinfo.value.idempotency_key + assert key is not None + + server.state.model_run_error = None + replayed = await client.models.run(MODEL, ARGS, idempotency_key=key) + + assert isinstance(replayed, BinaryResult) + assert replayed.content == AUDIO + assert server.state.model_run_generations == 1 + + +def test_a_retry_that_succeeds_binary_returns_the_bytes(server) -> None: + # The retry loop returns whatever the successful attempt produced; nothing + # about the binary branch is outside it. + _serve_audio(server) + server.state.model_run_fail_times = 1 + server.state.model_run_transient_error = (429, "queue_full") + server.state.model_run_retry_after = "0" + with Comfy() as client: + result = client.models.run(MODEL, ARGS) + assert isinstance(result, BinaryResult) + assert result.content == AUDIO + assert server.state.model_run_count == 2 + + +# --- the result type itself ---------------------------------------------- + + +def test_binary_result_is_frozen_and_exposes_exactly_three_fields() -> None: + result = BinaryResult(content=b"abc", content_type="audio/mpeg", request_id="req_1") + assert [f.name for f in dataclasses.fields(result)] == [ + "content", + "content_type", + "request_id", + ] + with pytest.raises(dataclasses.FrozenInstanceError): + result.content = b"xyz" # type: ignore[misc] + + +def test_binary_result_repr_does_not_dump_the_body() -> None: + # This object holds a whole audio file and lands in tracebacks, REPL echoes + # and CI logs; a dataclass's default repr would print every byte of it. + result = BinaryResult(content=b"\xff\xfb" * 100_000, content_type="audio/mpeg", request_id=None) + text = repr(result) + assert "200000 bytes" in text + assert "audio/mpeg" in text + assert len(text) < 200 + + +def test_binary_results_compare_by_value() -> None: + a = BinaryResult(content=b"abc", content_type="audio/mpeg", request_id="req_1") + b = BinaryResult(content=b"abc", content_type="audio/mpeg", request_id="req_1") + assert a == b + assert a != dataclasses.replace(a, content=b"abd") + + +def test_binary_result_is_exported_from_both_layers() -> None: + # A caller needs the name to write `isinstance(result, BinaryResult)`, so + # it is part of the public surface rather than an implementation detail of + # the transport. + import comfy_low + import comfy_sdk + import comfy_sdk.models + + assert comfy_sdk.BinaryResult is BinaryResult + assert comfy_sdk.models.BinaryResult is BinaryResult + assert comfy_low.BinaryResult is BinaryResult + assert "BinaryResult" in comfy_sdk.__all__ + assert "BinaryResult" in comfy_low.__all__ + + +# --- the media-type predicate the branch is built on ---------------------- + + +@pytest.mark.parametrize( + ("header", "expected"), + [ + ("application/json", "application/json"), + ("application/json; charset=utf-8", "application/json"), + (" Application/JSON ;charset=UTF-8", "application/json"), + ("audio/mpeg", "audio/mpeg"), + ("audio/L16; rate=16000", "audio/l16"), + # `httpx.Headers.get` joins a header sent twice with ", ", so a comma + # ends the type as surely as a semicolon does. + ("application/json, application/json", "application/json"), + ("application/json;charset=utf-8, application/json", "application/json"), + ("audio/mpeg, audio/mpeg", "audio/mpeg"), + ("", ""), + (None, ""), + ], +) +def test_media_type_strips_parameters_and_case(header: str | None, expected: str) -> None: + assert media_type(header) == expected + + +@pytest.mark.parametrize( + ("media", "expected"), + [ + ("application/json", True), + ("application/vnd.acme.result+json", True), + ("application/ld+json", True), + ("audio/mpeg", False), + ("text/html", False), + ("text/plain", False), + ("application/octet-stream", False), + # Not JSON: `application/jsonlines` merely starts with the same string, + # and a `startswith` test would have decoded it as a document. + ("application/jsonlines", False), + ("", False), + ], +) +def test_is_json_media_type_matches_the_declared_branch(media: str, expected: bool) -> None: + assert is_json_media_type(media) is expected diff --git a/tests/test_router_spec_contract.py b/tests/test_router_spec_contract.py index 4e51f76..efda9ed 100644 --- a/tests/test_router_spec_contract.py +++ b/tests/test_router_spec_contract.py @@ -274,6 +274,55 @@ def test_the_bound_path_has_exactly_the_two_segments_the_binding_fills() -> None assert "{model}" in _MODEL_RUN_PATH_TEMPLATE +# --- the two media types the run route's 200 can answer under ------------ + + +def _run_200_content() -> dict[str, Any]: + """The ``content`` map of ``runRouterModel``'s ``200``, read out of the spec.""" + doc = yaml.safe_load(ROUTER_SPEC.read_text(encoding="utf-8")) + item = (doc.get("paths") or {})[_MODEL_RUN_PATH_TEMPLATE] + content = item["post"]["responses"]["200"]["content"] + assert isinstance(content, dict) and content, "runRouterModel's 200 declares no content" + return content + + +def test_the_run_200_declares_both_a_json_and_a_binary_branch() -> None: + """The contract behind ``post_model_run`` returning ``dict | BinaryResult``. + + Read out of the spec rather than restated, for the same reason the route is: + the day a sync drops the ``*/*`` branch (or adds a third one), the SDK's + two-way branch is either dead code or newly incomplete, and nothing else in + the suite would notice — the binary tests drive a *stub*, which asserts the + SDK's behaviour rather than the server's contract. + """ + content = _run_200_content() + assert set(content) == {"application/json", "*/*"}, ( + f"the vendored spec's runRouterModel 200 declares {sorted(content)}; " + "comfy_low.transport._Prepared.parse_run_result branches on exactly two " + "cases (JSON -> dict, anything else -> BinaryResult)" + ) + + +def test_the_binary_branch_is_declared_as_raw_bytes() -> None: + # `format: binary` is what says the body is bytes rather than a base64 + # string or a JSON document — i.e. that `BinaryResult.content` is the + # partner's file and needs no decoding on the way out. + schema = _run_200_content()["*/*"].get("schema") or {} + assert schema.get("type") == "string" + assert schema.get("format") == "binary" + + +def test_the_200_promises_the_headers_a_binary_result_is_built_from() -> None: + # `BinaryResult.request_id` reads `X-Comfy-Request-Id` off a *success*, and + # the SDK takes the partner's `Content-Type` at its word — which is only + # safe because the route sends `X-Content-Type-Options: nosniff`. + doc = yaml.safe_load(ROUTER_SPEC.read_text(encoding="utf-8")) + headers = doc["paths"][_MODEL_RUN_PATH_TEMPLATE]["post"]["responses"]["200"]["headers"] + assert "X-Comfy-Request-Id" in headers + assert "X-Content-Type-Options" in headers + assert "Idempotent-Replayed" in headers + + # --- run_detailed's header lifts, pinned against the contract ----------------- # # `RouterRunResult` is built entirely out of response header names. A name is @@ -296,10 +345,6 @@ def test_the_bound_path_has_exactly_the_two_segments_the_binding_fills() -> None "request_id": "X-Comfy-Request-Id", } -#: Lifted by the SDK but NOT declared on the contract's 200 -- see the tripwire -#: test at the bottom of this file. -_UNDECLARED_HEADER_LIFTS = {"credits_used": "X-Comfy-Credits-Used"} - def _declared_run_response_headers() -> set[str]: """The header names the spec declares on ``runRouterModel``'s ``200``.""" @@ -340,27 +385,21 @@ def test_the_lift_actually_reads_the_declared_name(field: str, header: str) -> N ) -@pytest.mark.parametrize(("field", "header"), sorted(_UNDECLARED_HEADER_LIFTS.items())) -def test_an_undeclared_lift_stays_undeclared_until_someone_reconciles_it( - field: str, header: str -) -> None: - """Tripwire, and deliberately asserting the *absence*. - - ``credits_used`` is lifted from a header the vendored contract does not - declare anywhere -- the 200's only cost headers are the - ``X-Committed-Spend-*`` trio, which is a different quantity (USD cents of - in-flight commitment, not the price of this run). Nothing in the suite can - catch a wrong name here, because every test configures its stub to emit the - exact literal the lift reads. - - That gap is tracked, not accepted. This test fails the moment a spec sync - declares the header, which is the signal to move the entry up into - ``_CONTRACT_HEADER_LIFTS`` and get it pinned like the rest. It also fails - if the header is declared under a *different* name for the same quantity, - because the reconciliation is the same either way. +def test_credits_used_header_is_declared_by_the_contract() -> None: + """``credits_used`` is pinned to its declared name here, not above. + + It shares ``test_every_lifted_header_is_declared_by_the_contract``'s + declared-ness check, but not ``test_the_lift_actually_reads_the_declared_name``: + that test proves a lift reads a header by checking presence changes the + field's value, and does so with the literal ``"x"`` -- which + :func:`_credits_used` rejects as not a finite decimal, so it would come + back ``None`` whether or not the header were sent. `tests/test_models_run.py` + already pins the reads-the-declared-name half of this contract with values + that actually parse. """ declared = _declared_run_response_headers() - assert header not in declared, ( - f"the vendored spec now declares {header!r}: move {field!r} from " - f"_UNDECLARED_HEADER_LIFTS into _CONTRACT_HEADER_LIFTS so it is pinned." + assert "X-Comfy-Credits-Used" in declared, ( + "RouterRunResult.credits_used is lifted from 'X-Comfy-Credits-Used', which the " + f"vendored spec does not declare on runRouterModel's 200. Declared: {sorted(declared)}. " + "Either a sync renamed the header or the SDK is reading a name Router never sends." )