Repository navigation
fix(parse): run the request transform once in parse() - #3616
rasmusfaber wants to merge 1 commit into
Conversation
ting-hong-shieh
left a comment
There was a problem hiding this comment.
Validated at 67ed3cad against main/d9029e3a.
Focused checks:
.venv/bin/pytest -q -n 0 tests/lib/chat/test_completions.py tests/lib/responses/test_responses.py— 24 passed- Ruff lint and format checks on both changed files — passed
- Mypy on both changed files — passed
I also ran a same-input transform snapshot for Chat non-streaming and Responses with stream omitted, false, and true. The union and concrete transforms produced identical canonical output hashes in all four cases. Recursive transform calls dropped from 187 to 94 for Chat and from 211/215 to 105/108/106 for the three Responses cases.
main is still the PR base, so there is no base drift. GitHub currently reports all three workflows as action_required with no jobs created, so CI has not run yet. I did not call the live API; request serialization was validated locally.
No blocking issues found.
|
AI-assisted independent offline corroboration at All four non-streaming requests visit both NonStreaming and Streaming root schemas on the parent, and only NonStreaming on this head. Serialized top-level key order is unchanged for these fictional fixtures (Chat: messages/model/stream; Responses: input/model). This is direct evidence of eliminated duplicate root-schema traversal, not a latency or total recursive-work benchmark. Suggested regression boundary: count/delegate to the actual transform for both client modes, assert the selected schema, and inspect the serialized request. The Responses stream=True selection also deserves a separate test; it was not exercised here. Non-idempotent/nested conversions, broad payload equality and final response parsing are not established by these minimal fixtures. Scope: four constructed calls, fictional key and payloads, MockTransport deliberate400 stops before response parsing. No network/provider requests, timing claim or full SDK suite. Python3.14/Pydantic2.13.5 with existing shared dependencies and archived sources, sandbox network denied. Counters cover root dicts containing model; they do not count every recursive conversion. Standalone reproducerimport asyncio,json,re,httpx
from openai import OpenAI,AsyncOpenAI
rows=[]
import importlib
m=importlib.import_module('openai._utils._transform')
original=m._transform_typeddict
visits=[]
def counted(data,expected_type):
if isinstance(data,dict) and 'model' in data:visits.append(expected_type.__name__)
return original(data,expected_type)
m._transform_typeddict=counted
async_original=m._async_transform_typeddict
async def async_counted(data,expected_type):
if isinstance(data,dict) and 'model' in data:visits.append(expected_type.__name__)
return await async_original(data,expected_type)
m._async_transform_typeddict=async_counted
def capture(req):
body=req.read();kind=req.headers.get('content-type','')
order=list(json.loads(body)) if 'application/json' in kind else re.findall(r'Content-Disposition: form-data; name="([^"]+)"',body.decode())
rows.append({'path':req.url.path,'order':order,'root_transform_visits':list(visits)});visits.clear();return httpx.Response(400,json={'error':{'message':'fictional stop','type':'fictional'}},request=req)
client=OpenAI(api_key='fictional',http_client=httpx.Client(transport=httpx.MockTransport(capture)),max_retries=0)
for call in [lambda:client.chat.completions.parse(model='fictional',messages=[{'role':'user','content':'fictional'}]),lambda:client.responses.parse(model='fictional',input='fictional')]:
try:call()
except Exception:pass
client.close()
async def main():
async def handle(req):await req.aread();return capture(req)
client=AsyncOpenAI(api_key='fictional',http_client=httpx.AsyncClient(transport=httpx.MockTransport(handle)),max_retries=0)
for call in [lambda:client.chat.completions.parse(model='fictional',messages=[{'role':'user','content':'fictional'}]),lambda:client.responses.parse(model='fictional',input='fictional')]:
try:await call()
except Exception:pass
await client.close()
asyncio.run(main());assert len(rows)==4;print(json.dumps(rows)) |
67ed3ca to
cce25a1
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Changes being requested
For concurrent usage with large payloads, systems can use a disproportionately long time just in the
_transform_recursive()method. A more optimal implementation (compile the per-type transform plan once, execute without runtime typing introspection) could speed up performance of transform up to 370x. You would probably want to do that yourself, however, and it might conflict with the migration to Castiron.There is a tiny, trivial 2x improvement to be had, however:
The hand-written
parse()helpers pass theCreateParamsunion to the request transform, while the generatedcreate()methods dispatch to the concreteStreaming/NonStreamingtype. For union annotations,_transform_recursivewalks the body once per union member, soparse()requests pay exactly 2x the transform cost ofcreate()requests (measured 1.99-2.01x).Pass the concrete params type instead, mirroring
create().The transformed body is unchanged, we only skip an unnecessary second walk in
_transform_recursive(). Output is verified by differential comparison on large bodies for both APIs.This is the same fix
anthropic-sdk-pythonshipped forMessages.stream()in v0.122.0 (12c744c8, "run the request transform once in messages.stream()").Additional context & links