feat(models): add maxCompletionTokens for OpenAI#2175
Conversation
OpenAI's `max_tokens` request parameter is deprecated for the Chat Completions API and is rejected outright by reasoning models (GPT-5 / o-series): 400 Bad Request: Unsupported parameter: 'max_tokens' is not supported with this model. Use 'max_completion_tokens' instead. kagent's OpenAIConfig only exposed maxTokens (-> max_tokens), so there was no way to cap output for those models. Add a sibling maxCompletionTokens field that maps to the OpenAI `max_completion_tokens` parameter. maxTokens is left intact and unchanged for backward compatibility and for OpenAI-compatible endpoints (baseUrl) that still expect max_tokens; the two are independent, so existing configs are unaffected. Wired end to end: - add MaxCompletionTokens to OpenAIConfig (v1alpha2, +kubebuilder Minimum=1) and regenerate the CRDs - carry it on the adk.OpenAI serialization struct - copy Spec.OpenAI.MaxCompletionTokens in the controller translator - emit max_completion_tokens in the Python ADK OpenAI client and the Go ADK OpenAI model client Adds Go translator + Go ADK tests and a Python client test. Signed-off-by: anthonytwh <anthony.tam@tigera.io>
There was a problem hiding this comment.
Pull request overview
This PR adds support for OpenAI’s max_completion_tokens parameter end-to-end (CRD/API → controller translation → ADK serialization → Python/Go clients), enabling token caps on reasoning models that reject the deprecated max_tokens.
Changes:
- Introduces
maxCompletionTokensin the KubernetesOpenAIConfig(v1alpha2) with validation and regenerated CRDs/Helm templates. - Wires the new field through the controller translator and ADK model serialization for Go.
- Adds Python and Go client support (plus tests) to emit
max_completion_tokenson OpenAI chat completion requests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/kagent-adk/tests/unittests/models/test_openai.py | Adds a unit test asserting max_completion_tokens is forwarded and max_tokens is absent. |
| python/packages/kagent-adk/src/kagent/adk/types.py | Extends the Python OpenAI config model and config→LLM factory to include max_completion_tokens. |
| python/packages/kagent-adk/src/kagent/adk/models/_openai.py | Adds request serialization of max_completion_tokens in the OpenAI client wrapper. |
| helm/kagent-crds/templates/kagent.dev_modelconfigs.yaml | Updates Helm-rendered CRD schema/docs for maxCompletionTokens and clarifies maxTokens deprecation. |
| go/core/internal/controller/translator/agent/adk_api_translator.go | Copies Spec.OpenAI.MaxCompletionTokens into the ADK OpenAI model config. |
| go/core/internal/controller/translator/agent/adk_api_translator_test.go | Adds coverage verifying maxCompletionTokens propagates and maxTokens remains unset when not configured. |
| go/api/v1alpha2/modelconfig_types.go | Adds MaxCompletionTokens to the CRD API type with Minimum=1 validation and updated field docs. |
| go/api/config/crd/bases/kagent.dev_modelconfigs.yaml | Regenerates base CRD schema/docs to include maxCompletionTokens. |
| go/api/adk/types.go | Extends the ADK OpenAI serialization struct to include max_completion_tokens. |
| go/adk/pkg/models/openai.go | Extends the Go ADK OpenAI config struct to carry MaxCompletionTokens. |
| go/adk/pkg/models/openai_adk.go | Applies MaxCompletionTokens to openai-go ChatCompletionNewParams. |
| go/adk/pkg/models/openai_adk_test.go | Adds a unit test for applyOpenAIConfig setting MaxCompletionTokens. |
| go/adk/pkg/agent/agent.go | Threads MaxCompletionTokens from the ADK model into the Go OpenAI config used by the runtime. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.max_tokens: | ||
| kwargs["max_tokens"] = self.max_tokens | ||
| if self.max_completion_tokens: | ||
| kwargs["max_completion_tokens"] = self.max_completion_tokens |
| if cfg.MaxTokens != nil { | ||
| params.MaxTokens = openai.Int(int64(*cfg.MaxTokens)) | ||
| } | ||
| if cfg.MaxCompletionTokens != nil { | ||
| params.MaxCompletionTokens = openai.Int(int64(*cfg.MaxCompletionTokens)) | ||
| } |
…xclusive Addresses review feedback: if a ModelConfig set both maxTokens and maxCompletionTokens, the client sent both request parameters. They are mutually exclusive on the OpenAI API — reasoning models (GPT-5 / o-series) reject max_tokens, and some OpenAI-compatible endpoints only accept max_tokens — so sending both risks a hard 400 and makes migration brittle. Send exactly one at request-build time, with max_completion_tokens (the modern superset parameter) taking precedence when both are configured, in both the Python and Go ADK OpenAI clients. Single-field configs are unaffected. Adds precedence tests for both runtimes. Signed-off-by: anthonytwh <anthony.tam@tigera.io>
|
Good catch @copilot — fixed in 0b813ae. I went with modern-parameter precedence rather than the If you'd prefer an even stronger guard, I can also add a CRD-level CEL validation rejecting configs that set both fields — happy to include that if maintainers want it. |
supreme-gg-gg
left a comment
supreme-gg-gg
left a comment
There was a problem hiding this comment.
Makes sense overall, one comment before we can merge this
| // (GPT-5 / o-series) require in place of the deprecated maxTokens. | ||
| // +optional | ||
| // +kubebuilder:validation:Minimum=1 | ||
| MaxCompletionTokens int `json:"maxCompletionTokens,omitempty"` |
There was a problem hiding this comment.
We should add a validation rule that only one of MaxCompletionTokens or MaxTokens is provided (if I understood properly from the description they should be mutually exclusive)?
There was a problem hiding this comment.
Done in 9283c15. Added a type-level CEL rule on OpenAIConfig:
+kubebuilder:validation:XValidation:message="maxTokens and maxCompletionTokens are mutually exclusive",rule="!(has(self.maxTokens) && has(self.maxCompletionTokens))"
So a ModelConfig that sets both is now rejected at admission (matching the existing XValidation pattern used for the TLSConfig rules in this file). The runtime-level precedence guard in the Python/Go clients stays as defense-in-depth for OpenAI-compatible paths that don't go through CRD admission. Added an envtest-based CEL test (modelconfig_cel_test.go) covering both-set rejection plus the single-field and neither-field cases.
seeing one llm's output pitted against another's looks quite funny from the outside :) |
| // parameter, which is deprecated and rejected by reasoning models | ||
| // (GPT-5 / o-series). For those models set maxCompletionTokens instead. | ||
| // +optional | ||
| MaxTokens int `json:"maxTokens,omitempty"` |
There was a problem hiding this comment.
maxTokens has no +kubebuilder:validation:Minimum=1 marker but maxCompletionTokens below does..
value like maxTokens: 0 or maxTokens: -1 passes crd validation silently.. and is then ignored by the translator
adding the same minimum marker here would make the two fields consistent in my oprinio
There was a problem hiding this comment.
Good catch — fixed in 9283c15. maxTokens now carries +kubebuilder:validation:Minimum=1 too, so maxTokens: 0/-1 is rejected at admission instead of silently ignored by the translator. The regenerated CRD bases/helm templates reflect minimum: 1 on both fields, and the new CEL test asserts a negative value on either field is rejected.
There was a problem hiding this comment.
Good catch — fixed in 9283c15.
maxTokensnow carries+kubebuilder:validation:Minimum=1too, somaxTokens: 0/-1is rejected at admission instead of silently ignored by the translator. The regenerated CRD bases/helm templates reflectminimum: 1on both fields, and the new CEL test asserts a negative value on either field is rejected.
why do you post llm outputs here instead of writing your own sentences? i think everyone should maintain healthy communication as a sign of respect for the person they are interacting with..
There was a problem hiding this comment.
It is automatic, no disrespect here.. Thanks for your comment on the PR.
…usivity and minimum Add a type-level CEL validation rule rejecting ModelConfigs that set both maxTokens and maxCompletionTokens, and add the missing Minimum=1 marker to maxTokens so non-positive values are rejected at admission rather than silently ignored by the translator. Regenerate CRD bases and helm templates, and add an envtest-based CEL test covering both-set rejection, minimum enforcement, and single-field configs. Signed-off-by: anthonytwh <anthony.tam@tigera.io>
EItanya
left a comment
EItanya
left a comment
There was a problem hiding this comment.
LGTM, I'll leave to @supreme-gg-gg for final approval
What
Add a
maxCompletionTokensfield to the OpenAIModelConfigthat maps to OpenAI'smax_completion_tokensrequest parameter.Why
OpenAI's
max_tokensparameter is deprecated for the Chat Completions API and is rejected outright by reasoning models (GPT-5 / o-series):Today
OpenAIConfigonly exposesmaxTokens(→max_tokens), so there is no way to cap output tokens for GPT-5 / o-series models — settingmaxTokenson such a model makes every request fail. (Per OpenAI's SDK docstrings,max_tokens"is now deprecated in favor ofmax_completion_tokens, and is not compatible with o-series models";max_completion_tokensis "an upper bound … including visible output tokens and reasoning tokens".)Approach — additive, non-breaking
maxTokensis left intact and unchanged:BaseOpenAIalso servesbaseUrl-overridden OpenAI-compatible servers, some of which still expectmax_tokens. Keeping both lets operators choose the right one per model.The two fields are independent; set
maxCompletionTokensfor reasoning models,maxTokensfor legacy/compatible endpoints.Changes
OpenAIConfig.MaxCompletionTokens(v1alpha2),+kubebuilder:validation:Minimum=1; CRDs regenerated (bases + Helm).adk.OpenAIserialization struct carriesmax_completion_tokens.Spec.OpenAI.MaxCompletionTokens.max_completion_tokens;OpenAIconfig model gains the field (ge=1).applyOpenAIConfig+CreateLLM) emitsmax_completion_tokensvia the openai-go SDK.Tests
Test_AdkApiTranslator_OpenAIMaxCompletionTokens— verifies the field flows through andmax_tokensstays unset.TestApplyOpenAIConfig/config_with_max_completion_tokens.test_generate_content_async_with_max_completion_tokens— assertsmax_completion_tokensis forwarded andmax_tokensis absent.All pass;
go build ./api/... ./adk/... ./core/internal/controller/...is green and CRD generation is clean.Note
Scope is the native OpenAI provider (the one that hits
api.openai.com). Azure OpenAI reasoning deployments have the same underlying need; happy to extendAzureOpenAIConfigin a follow-up if maintainers prefer.