feat(py/plugins/google-genai): raw Interactions HTTP client, converters, and options - #5855
feat(py/plugins/google-genai): raw Interactions HTTP client, converters, and options#5855cabljac wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Google AI Interactions API within the genkit-google-genai package, adding HTTP client helpers, message converters, configuration options, and utility functions, along with corresponding unit tests. It also bumps the google-genai dependency requirement to version 2.14.0. The review feedback highlights three key issues: a bug in ensure_tool_ids that can cause tool request and response ID mismatches when some requests already have a ref, a control flow anti-pattern in cancel_interaction where an exception is raised on a successful path, and a potential crash in status_from_api_error if error.code is non-numeric.
| def ensure_tool_ids(messages: list[Message]) -> list[Message]: | ||
| """Assign stable tool call IDs so wire payloads stay pairable.""" | ||
| generated_ids: list[str] = [] | ||
| next_id_counter = 0 | ||
|
|
||
| new_messages = [message.model_copy(deep=True) for message in messages] | ||
|
|
||
| for message in new_messages: | ||
| for part in message.content: | ||
| root = part.root | ||
| if isinstance(root, ToolRequestPart) and root.tool_request and not root.tool_request.ref: | ||
| new_id = f'genkit-auto-id-{next_id_counter}' | ||
| next_id_counter += 1 | ||
| root.tool_request.ref = new_id | ||
| generated_ids.append(new_id) | ||
|
|
||
| # Responses without refs reuse request IDs in order; unmatched ones get orphan IDs. | ||
| for message in new_messages: | ||
| for part in message.content: | ||
| root = part.root | ||
| if isinstance(root, ToolResponsePart) and root.tool_response and not root.tool_response.ref: | ||
| matched_id = generated_ids.pop(0) if generated_ids else None | ||
| if matched_id: | ||
| root.tool_response.ref = matched_id | ||
| else: | ||
| root.tool_response.ref = f'genkit-orphan-id-{next_id_counter}' | ||
| next_id_counter += 1 | ||
|
|
||
| return new_messages |
There was a problem hiding this comment.
There is a bug in how tool request and response IDs are matched when some requests already have a ref while others do not.
Currently, generated_ids only stores the auto-generated IDs for requests that did not have a ref. When processing responses, any response without a ref pops from generated_ids in order. If a request earlier in the list already had a ref, the first response without a ref (which corresponds to that first request) will incorrectly consume the auto-generated ID of the second request, causing a complete mismatch of tool IDs.
To fix this, we should track the refs of all requests in order, and safely remove matched refs when processing responses that already have them.
def ensure_tool_ids(messages: list[Message]) -> list[Message]:
'''Assign stable tool call IDs so wire payloads stay pairable.'''
generated_ids: list[str] = []
next_id_counter = 0
new_messages = [message.model_copy(deep=True) for message in messages]
for message in new_messages:
for part in message.content:
root = part.root
if isinstance(root, ToolRequestPart) and root.tool_request:
if not root.tool_request.ref:
new_id = f'genkit-auto-id-{next_id_counter}'
next_id_counter += 1
root.tool_request.ref = new_id
generated_ids.append(root.tool_request.ref)
# Responses without refs reuse request IDs in order; unmatched ones get orphan IDs.
for message in new_messages:
for part in message.content:
root = part.root
if isinstance(root, ToolResponsePart) and root.tool_response:
if not root.tool_response.ref:
matched_id = generated_ids.pop(0) if generated_ids else None
if matched_id:
root.tool_response.ref = matched_id
else:
root.tool_response.ref = f'genkit-orphan-id-{next_id_counter}'
next_id_counter += 1
elif root.tool_response.ref in generated_ids:
generated_ids.remove(root.tool_response.ref)
return new_messages| try: | ||
| await request( | ||
| method='POST', | ||
| url=url, | ||
| api_key=api_key, | ||
| client_options=client_options, | ||
| ) | ||
| # Successful cancel often surfaces as CANCELLED; normalize both paths. | ||
| raise GenkitError(status='CANCELLED', message='successfully cancelled') | ||
| except GenkitError as error: | ||
| if error.status == 'CANCELLED': | ||
| return Interaction.model_validate({'id': interaction_id, 'status': 'cancelled'}) | ||
| raise |
There was a problem hiding this comment.
Raising an exception (GenkitError) on the successful path just to catch it and return a normalized Interaction is a control flow anti-pattern. It also discards the actual successful response returned by the server, which may contain valuable metadata or fields.
Instead, we can return the response directly on success (optionally copying it to normalize the status to 'cancelled'), and only catch GenkitError with status 'CANCELLED' for cases where the server returned an error that represents a successful cancellation.
| try: | |
| await request( | |
| method='POST', | |
| url=url, | |
| api_key=api_key, | |
| client_options=client_options, | |
| ) | |
| # Successful cancel often surfaces as CANCELLED; normalize both paths. | |
| raise GenkitError(status='CANCELLED', message='successfully cancelled') | |
| except GenkitError as error: | |
| if error.status == 'CANCELLED': | |
| return Interaction.model_validate({'id': interaction_id, 'status': 'cancelled'}) | |
| raise | |
| try: | |
| res = await request( | |
| method='POST', | |
| url=url, | |
| api_key=api_key, | |
| client_options=client_options, | |
| ) | |
| return res.model_copy(update={'status': 'cancelled'}) | |
| except GenkitError as error: | |
| if error.status == 'CANCELLED': | |
| return Interaction.model_validate({'id': interaction_id, 'status': 'cancelled'}) | |
| raise |
| } | ||
| if candidate in valid: | ||
| return cast(StatusName, candidate) | ||
| return status_for_http_code(int(error.code or 0)) |
There was a problem hiding this comment.
If error.code is a non-numeric string (e.g., a status name or string representation of an error), calling int(error.code) will raise a ValueError and crash the error mapping process. This is particularly problematic because it masks the original API error and makes debugging difficult.
We should wrap the conversion in a try-except block to safely fall back to 0 if parsing fails.
| return status_for_http_code(int(error.code or 0)) | |
| try: | |
| code = int(error.code or 0) | |
| except (TypeError, ValueError): | |
| code = 0 | |
| return status_for_http_code(code) |
4cc1a39 to
5013ae9
Compare
…rs, and options Transport layer extracted from #5806: minimal async HTTP client for the Google AI Interactions API, request/response converters between Genkit and Interactions wire types, ClientOptions resolution, and shared error-mapping utilities. Bumps google-genai to >=2.14.0,<3 for the Interactions SDK types. Co-authored-by: Jeff Huang <huangjeff@google.com>
5013ae9 to
e079a08
Compare
| # Inline data URLs travel as base64; anything else is a reference the API fetches. | ||
| url = part.media.url | ||
| if url.startswith('data:'): | ||
| block['data'] = url[url.index(',') + 1 :] |
There was a problem hiding this comment.
I think url[url.index(',') + 1:] can throw a bare ValueError: substring not found if a data URL arrives without a comma. The other failure modes in this function all raise descriptive errors, so a malformed media part could plausibly be a confusing one to debug. Something like:
if ',' not in url:
raise ValueError('Malformed data URL for media part: missing payload separator')
block['data'] = url.split(',', 1)[1]| raise RuntimeError( | ||
| 'Multimodal embedding relies on google-genai client internals that are ' | ||
| 'unavailable in the installed google-genai version; install google-genai>=1.63.0.' | ||
| 'unavailable in the installed google-genai version; install google-genai>=2.3.0.' |
There was a problem hiding this comment.
This message suggests google-genai>=2.3.0 but the floor this PR sets in pyproject is >=2.14.0,<3, so the suggested install can't satisfy the guard. The test in googlegenai_embedder_test.py asserts the same string. Both should move to 2.14.0, unless 2.3.0 is the real feature floor for the private transport.
Second slice of the #5806 split (stacked on #5854): minimal async HTTP client for the Google AI Interactions API, request/response converters between Genkit and Interactions wire types, ClientOptions resolution, shared error-mapping utilities, and the google-genai >=2.14.0,<3 bump.
Part of the stack splitting #5806; recombined it reproduces that PR byte-for-byte. Co-authored with @huangjeff5.
Breaking changes
google-genaidependency floor moves from>=1.63.0to>=2.14.0,<3(major version bump) for the Interactions SDK types. Downstream projects pinned below 2.x will fail to resolve.