feat: improve gemini thinking budget adaption - #1247
Conversation
WalkthroughThis change introduces a helper function to centralize and standardize the logic for clamping the "thinking budget" parameter for different Gemini model variants. The refactoring updates how model name suffixes related to thinking budgets are parsed and validated, ensuring consistent handling across model types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CovertGemini2OpenAI
participant clampThinkingBudget
Client->>CovertGemini2OpenAI: Request with model name (may include -thinking- or -nothinking)
CovertGemini2OpenAI->>clampThinkingBudget: Validate and clamp thinking budget for model
clampThinkingBudget-->>CovertGemini2OpenAI: Return clamped budget
CovertGemini2OpenAI-->>Client: Proceed with validated budget
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
relay/channel/gemini/relay-gemini.go (3)
42-46: Prefer clearer, model-scoped constant names
pro25MinBudget,flash25MaxBudget, etc. are easy to confuse across branches. Prefixing with the model family (e.g.gemini25ProMinBudget) reduces the mental mapping cost and prevents accidental cross-use later.-const ( - pro25MinBudget = 128 +const ( + gemini25ProMinBudget = 128 … )
101-108: Model-variant detection duplicated
isNew25Pro/is25FlashLiteare recalculated here although identical logic exists insideclampThinkingBudget. Consider re-using a shared helper to avoid subtle divergence when the list of preview models changes.
119-144: Round percentage-derived budgets instead of truncatingCasting the float → int with
int(budgetTokens)always floors the value. For small budgets that can drop the limit by almost one token batch. Usemath.Round(ormath.Ceil) for clearer intent:-import "strconv" +import ( + "strconv" + "math" +) // … - budgetTokens := model_setting.GetGeminiSettings().ThinkingAdapterBudgetTokensPercentage * - float64(geminiRequest.GenerationConfig.MaxOutputTokens) - clampedBudget := clampThinkingBudget(modelName, int(budgetTokens)) + raw := model_setting.GetGeminiSettings().ThinkingAdapterBudgetTokensPercentage * + float64(geminiRequest.GenerationConfig.MaxOutputTokens) + clampedBudget := clampThinkingBudget(modelName, int(math.Round(raw)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
relay/channel/gemini/relay-gemini.go(3 hunks)
🔇 Additional comments (2)
relay/channel/gemini/relay-gemini.go (2)
108-118: Validate parsed budget before clampingIf the suffix contains a non-positive number (e.g.
-thinking--5or-thinking-0), we silently coerce it to the model’s minimum. Returning a user-visible error would surface mis-configuration earlier and keeps API behaviour explicit.
144-150: Blocking-nothinkingfor flash-lite looks correctExtending the restriction beyond 2.5-pro to include
2.5-flash-litealigns with the published limits. No issues spotted.
| // clampThinkingBudget 根据模型名称将预算限制在允许的范围内 | ||
| func clampThinkingBudget(modelName string, budget int) int { | ||
| isNew25Pro := strings.HasPrefix(modelName, "gemini-2.5-pro") && | ||
| !strings.HasPrefix(modelName, "gemini-2.5-pro-preview-05-06") && | ||
| !strings.HasPrefix(modelName, "gemini-2.5-pro-preview-03-25") | ||
| is25FlashLite := strings.HasPrefix(modelName, "gemini-2.5-flash-lite") | ||
|
|
||
| if is25FlashLite { | ||
| if budget < flash25LiteMinBudget { | ||
| return flash25LiteMinBudget | ||
| } | ||
| if budget > flash25LiteMaxBudget { | ||
| return flash25LiteMaxBudget | ||
| } | ||
| } else if isNew25Pro { | ||
| if budget < pro25MinBudget { | ||
| return pro25MinBudget | ||
| } | ||
| if budget > pro25MaxBudget { | ||
| return pro25MaxBudget | ||
| } | ||
| } else { // 其他模型 | ||
| if budget < 0 { | ||
| return 0 | ||
| } | ||
| if budget > flash25MaxBudget { | ||
| return flash25MaxBudget | ||
| } | ||
| } | ||
| return budget | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
clampThinkingBudget mixes concerns & repeats model-detection logic
-
The
isNew25Pro/is25FlashLitechecks will soon live in at least three places (here, inCovertGemini2OpenAI, and very likely in request validation). A single helper such asdetectGeminiVariant(modelName) (variant enum)would avoid drift. -
For negative budgets the function silently upgrades to the model’s minimum (128 / 512). That might be desirable, but it is implicit. At minimum add a short doc-comment or, better, reject negatives and let the caller decide:
- if budget < pro25MinBudget {
- return pro25MinBudget
+ if budget <= 0 {
+ return pro25MinBudget // or: return 0, signalling “invalid”- The max for “other models” is
flash25MaxBudget; consider renaming that constant or adding a comment so the cap doesn’t look arbitrary.
Refactoring sketch (compressed):
type geminiVariant int
const (
varUnknown geminiVariant = iota
var25Pro
var25FlashLite
varOther
)
func detectVariant(m string) geminiVariant { … }
func clampBudget(v geminiVariant, b int) int { … }This removes string-prefix checks from every caller.
🤖 Prompt for AI Agents
In relay/channel/gemini/relay-gemini.go around lines 49 to 79, the
clampThinkingBudget function mixes model detection logic with budget clamping,
causing repeated string-prefix checks and implicit handling of negative budgets.
Refactor by extracting model detection into a separate helper function
detectGeminiVariant(modelName) returning a variant enum, then rewrite
clampThinkingBudget to accept this variant and budget, explicitly rejecting
negative budgets or documenting behavior clearly. Also, rename or comment the
flash25MaxBudget constant used as max for other models to clarify its purpose
and avoid confusion.
feat: improve gemini thinking budget adaption
2 participants
Summary by CodeRabbit
Refactor
Bug Fixes