Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to map the deprecated "max_tokens" parameter to "max_completion_tokens" for OpenAI reasoning models or when "reasoning_effort" is specified, while preserving "max_tokens" for legacy models. It also adds corresponding unit tests. The review feedback suggests making the reasoning model detection heuristic more robust to handle fine-tuned models (prefixed with "ft:") and custom deployment names, and updating the tests to cover these scenarios.
|
Addressed the Gemini review feedback in commit 1a26dcf. Reasoning-model detection now handles fine-tuned IDs (ft: prefix) , with regression coverage for both cases. |
|
Formatting-only follow-up pushed in commit The failed
The latest push triggered the PR workflows; CLA, Actions scan, and labeler have passed. The remaining PR workflows currently show |
| # both knobs are supplied so the request remains valid. | ||
| if config.max_completion_tokens is not None: | ||
| continue | ||
| if _uses_max_completion_tokens(model) or config.reasoning_effort is not None: |
There was a problem hiding this comment.
The reasoning_effort branch is not covered. Can we add one case with a model id the heuristic does not match (say 'my-deployment') and reasoning_effort set, asserting max_completion_tokens goes out?
| continue | ||
| value = getattr(config, name) | ||
| if value is not None: | ||
| if name == 'max_tokens': |
There was a problem hiding this comment.
Nit: This docstring still says everything else goes out under its field name. Can we add a line that max_tokens becomes max_completion_tokens for reasoning models?
| @pytest.mark.asyncio | ||
| async def test_get_openai_config_uses_max_completion_tokens_for_reasoning_models() -> None: | ||
| """Reasoning models reject the deprecated max_tokens request field.""" | ||
| for model_name in ('gpt-6-astra', 'ft:o1-mini:my-org:custom', 'my-o1-mini-deployment'): |
There was a problem hiding this comment.
Nit: This file already uses pytest.mark.parametrize; can we use it here so each id shows up as its own case?
2 participants
Summary
Fixes #6320.
OpenAIConfig(max_tokens=...)was forwarded asmax_tokensfor reasoning models, which OpenAI rejects. This maps the deprecated option tomax_completion_tokensfor reasoning-model IDs (o1,o3,o4,gpt-5*, andgpt-6*) while preservingmax_tokensfor legacy models.When both options are supplied, the explicit
max_completion_tokensvalue wins so mutually exclusive fields are not sent together.Tests
ruff checkandruff format --checkpass on changed files.image_tokensis omitted from itsPromptTokensDetailsmodel.dotpromptz-handlebarscurrently publishes no Windows wheel.Google CLA requirements in
CONTRIBUTING.mdnoted.