feat(models): add LlmRequest.to_generate_content_body() - #6935
Open
cnpierrepapi wants to merge 1 commit into
Open
feat(models): add LlmRequest.to_generate_content_body()#6935cnpierrepapi wants to merge 1 commit into
cnpierrepapi wants to merge 1 commit into
Conversation
LlmRequest.config is one flat GenerateContentConfig namespace, but its fields go to three places on the wire: 8 at the top level, 22 inside generationConfig, and 3 that are client-side only and rejected by the endpoint. Nothing on the type says which is which, and getting it wrong does not raise. A custom BaseLlm that buries tools in generationConfig gets a 200 back with no function call in it, because none was offered. Rather than document the split, delegate to the same converter google-genai uses for its own built-in models, so there is one copy of the mapping and it moves when GenerateContentConfig moves. A hand written list is a second copy that goes stale the moment the type gains a field. Also handles three things that bite anyone calling the converter raw: strips the private "_url" routing key, which is a 400 if sent as body; covers the model_selection_config -> modelConfig rename, the only field that changes name in transit; and re-raises genai's ValueError with the LlmRequest.config field that caused it, so the error points at the line that set it rather than at the converter. Closes google#6880
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Opening this at @surajksharma07's go-ahead on that thread, which also settled that the helper belongs on
LlmRequestin ADK rather than in genai.Problem:
LlmRequest.configis a single flatGenerateContentConfigwith 35 fields. On the wire those fields go to three different destinations, and nothing on the type says which is which:Getting the split wrong does not raise. A custom
BaseLlmthat flattensconfigby hand and buriestoolsinsidegenerationConfigstill gets a 200 with a well formed response in it. The tools are simply absent, so the model answers from memory instead of calling anything, and it reads as a model that chose not to call the tool.Solution:
LlmRequest.to_generate_content_body(), delegating to the same converters google-genai uses for its own built-in models (_GenerateContentParameters_to_vertex/_to_mldev) rather than keeping a hand written field list.The delegation is the point. A documented table or a hardcoded list is a second copy of the mapping, and it goes stale the moment
GenerateContentConfiggains a field — every customBaseLlmin the wild then silently drops that field. I can vouch for the rot personally: the field table I first posted on #6880 said 6 fields go top level. It is 8. I had missedmodelArmorConfigandserviceTier, and that list was less than a day old when I wrote it.Three edge cases are handled, since each would bite anyone calling the converters directly:
_urlkey for the genai client to build the path from. It is routing information, not body content, and sending it is a 400. Stripped.model_selection_configlands ingenerationConfigasmodelConfig. It is the only field that changes name in transit, so grepping your own config keys against the emitted body gives a false miss on exactly that one. Covered by a test.enable_enhanced_civic_answersraisesValueErrorin Vertex mode (Developer API only). Loud, which is right, but it surfaces from inside the genai converter and aBaseLlmauthor reading that traceback has no reason to connect it to a field they set several layers up. Re-raised with theLlmRequest.configfield name and the target API attached, chained viafrom excso the original is intact.The helper takes an
api_clientwhen the caller has one, and falls back to avertexaiflag otherwise, so it stays usable from aBaseLlmthat talks REST directly and never builds a genai client. Vertex and the Developer API differ in more than the URL — some fields exist on one and not the other — so that choice is explicit at the call site rather than inferred.Testing Plan
Unit Tests:
13 new tests in
tests/unittests/models/test_generate_content_body.py, run together with the existingtest_llm_request.py:Environment: Python 3.12,
google-genai2.18.1, againstmainat 2c9c00d.Coverage: tools landing top level rather than in
generationConfig;temperaturestaying ingenerationConfig; the three client-only fields never reaching the wire; the_urlstrip; the full 33-field set splitting 8/22/3 with nothing unaccounted for; themodel_selection_config→modelConfigrename; the Developer-API-only field being named in the error message; a field Vertex rejects being accepted on the Developer API; a stringsystem_instructionbecoming a content block; a request with no config at all; the method matching the free function; and the API mode being passed through.The one that matters most for the rot argument:
test_every_config_field_is_classifiedwalksGenerateContentConfig.model_fieldsand fails if a field appears that lands in none of the three buckets. If genai adds a field and this helper cannot place it, CI says so instead of a user finding out through a silently dropped parameter.Manual End-to-End (E2E) Tests:
Verified against live Vertex AI (
gemini-2.5-flash, ADC, us-central1) before this was written up — a config with 33 of the 35 fields populated (omittingresponse_json_schema, which is mutually exclusive withresponse_schema, andenable_enhanced_civic_answers, which is Developer-API only) pushed through the converter produced exactly the 8/22/3 split above, withtoolsat the top level and no field silently lost. The unit tests reproduce that split offline, so no credentials are needed to check it.Checklist
Additional context
Formatted with
pyink==25.12(the version pinned inpyproject.toml) using the repo's own[tool.pyink]config, run in apython:3.12-slimcontainer. All three files come backunchanged.
The Google CLA is signed.