feat: return raw bytes from call_runner for non-JSON responses - #51
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
stream=True is not the natural shape for one bounded call returning a 120 KB image, it is there because the buffered path still assumes a JSON object. Say so at the call site and point at the SDK change and the follow-up issue that let it go away. Refs #47, livepeer/livepeer-python-gateway#51 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stream=True is not the natural shape for one bounded call returning a 120 KB image, it is there because the buffered path still assumes a JSON object. Say so at the call site and point at the SDK change and the follow-up issue that let it go away. Refs #47, livepeer/livepeer-python-gateway#51 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@j0sh I did add tests but we don't have an agreed testing framework setup. I can also remove these for now and we add this later. |
…bytes (#45) nginx.conf.template is the whole runner, so the example's "no Livepeer code in the app" ends at no app code at all. runner.py and the Dockerfile are gone, and api-proxy leaves the CI image matrix. The config is the security model: it pins the model URL, the method, the credential and Accept, and strips the caller's query string, leaving the body as the only caller controlled input. The app id names the model, not the proxy, since callers match it exactly. The client reads raw JPEG bytes with call_runner(stream=True) until livepeer/livepeer-python-gateway#51 lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Is this streamable? If not, is that a regression? Eg. #25 |
|
Merged the latest The response refactor introduced an error-mapping regression: when a JSON response contained bytes that could not be decoded as a valid JSON encoding, The buffered response helper is now internal as Verification:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Content-type detection was a substring test (`"json" in content_type`), which is right for `+json` types only by accident and wrong for multi-document formats: `application/jsonl` (this repo's own trickle channel default), `application/x-ndjson`, and `application/json-seq` all matched and then failed in json.loads, so a working runner fronting a streaming API got "did not return valid JSON" instead of its bytes — the same bug class this branch set out to fix. Match on the media subtype instead, via aiohttp's own mimetype parser: `application/json` or the RFC 6839 `+json` structured suffix. Vendor types (`application/vnd.acme.v1+json`) keep parsing without being listed; multi-document bodies fall through to `raw`. Only four classifications change, all previously raising. Also fold the raw early return into the single existing return, so `payment_session=None if payment_type == "fixed" else payment_session` stays in one place — payment-type handling here has been reverted twice and there is no paid-binary test to catch the two sites drifting. The `session_id` expression reduces to the early return's behavior when `data` is empty, so this is equivalent. Add `content_type` to the JSON parse error: it is the fact that routed the response into parsing, and without it a misclassification is undebuggable from the message alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`content_type` is self-documenting from the field name; the invariant worth stating is that a populated `raw` means an empty `data`. The subtype helper matches its bare neighbors in this module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`raw` already means "the original JSON dict, un-normalized" on three dataclasses in this SDK — LiveRunnerSessionEvent.raw, LiveRunnerInstance.raw, LiveVideoToVideo.raw — and examples dump it with json.dumps(x.raw). A `raw: Optional[bytes]` on LiveRunnerCallResult overloads the name to mean the opposite, sharply so on one expression chain: `result.raw` (bytes) sitting one dot from `result.runner.raw` (dict). `content` matches its sibling `content_type` and the ecosystem convention for a response body as bytes (requests/httpx `.content`) — where `.raw` instead means an unread stream object, so the old name actively misled. Nothing consumes the field yet (runner-app-examples#45 and api-proxy both use the streaming path), so this is free now and permanent once released. Semantics unchanged: still `Optional[bytes] = None`, since `b""` is a valid empty body and `None` is the only unambiguous "this was JSON" sentinel. Also renames the local `raw` to `body` — it holds bytes, while `raw` in this codebase reads as a dict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5c061bb to
b97ab53
Compare
Closes the last gap after #25: the buffered
call_runnerpath required every response to be a JSON object, so binary-returning apps (an image, say — the hf-inference text-to-image API returns rawimage/jpegbytes) forced a base64-in-JSON shim into every runner and client. Streaming already handled bytes (stream=True→aiter_bytes()); this brings the buffered path to parity.What
http.request_data()— same request semantics and error mapping asrequest_json, but returns(body_bytes, content_type)without assuming JSON.request_jsonis now a thin wrapper over it (fetch + parse), behavior unchanged.call_runner(non-stream): single-document JSON —application/jsonor an RFC 6839+jsonsuffix — parses intoresult.dataexactly as before (including the strict object check andsession_idpickup). Any other content type returnsresult.content(bytes) +result.content_typeinstead of raising "expected JSON object".LiveRunnerCallResult: new optionalcontentandcontent_typefields;content_typeis also set on JSON results.contentisNonefor JSON —b""is a valid empty binary body, soNoneis the unambiguous "this was JSON" sentinel.Compatibility
Strictly additive: every response that succeeded before behaves identically. The only changed cases previously raised — a non-JSON content type now succeeds with bytes.
Content-type matching is by media subtype rather than a substring test, so vendor types (
application/problem+json,application/vnd.acme.v1+json) parse without being listed, while multi-document formats thatjson.loadscannot parse (application/jsonl— this repo's own trickle-channel default — plusapplication/x-ndjson,application/json-seq) return bytes instead of raising. All four flipped classifications were previously errors.One edge note: a server returning JSON under a non-JSON content type (e.g. bare
text/plain) lands incontentinstead of being parsed; aiohttp'sjson_response(what the examples use) always setsapplication/json.The field is named
content, notraw, becauserawalready means "the original JSON dict" onLiveRunnerSessionEvent,LiveRunnerInstance, andLiveVideoToVideo—result.contentwould otherwise sit one dot fromresult.runner.rawwith the opposite meaning. It also matches the requests/httpx convention for a body as bytes.Why
With this, a runner that fronts a bytes-returning API needs no transform at all — the example-apps
api-proxycan drop its base64 envelope, and a runner can literally be a stock nginxproxy_pass(see livepeer/runner-app-examples#45).Tested
tests/test_call_runner_raw.py(7 cases against a live aiohttp server): JSON unchanged incl.session_id,+jsonvendor type parsed, binary →content+content_type, ndjson →content, invalid JSON with a JSON content type still raises (and names the content type), JSON array still rejected, HTTP errors still raise. Full suite passes (16).