Skip to content

Commit 4b61405

Browse files
joshualittjacob314
authored andcommitted
feat(core): Late resolve GenerateContentConfigs and reduce mutation. (#14920)
1 parent f0a1722 commit 4b61405

File tree

7 files changed

+159
-158
lines changed

7 files changed

+159
-158
lines changed

packages/core/src/availability/policyHelpers.test.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -132,34 +132,46 @@ describe('policyHelpers', () => {
132132

133133
it('returns requested model if it is available', () => {
134134
const config = createExtendedMockConfig();
135+
mockModelConfigService.getResolvedConfig.mockReturnValue({
136+
model: 'gemini-pro',
137+
generateContentConfig: {},
138+
});
135139
mockAvailabilityService.selectFirstAvailable.mockReturnValue({
136140
selectedModel: 'gemini-pro',
137141
});
138142

139-
const result = applyModelSelection(config, 'gemini-pro');
143+
const result = applyModelSelection(config, { model: 'gemini-pro' });
140144
expect(result.model).toBe('gemini-pro');
141145
expect(result.maxAttempts).toBeUndefined();
142146
expect(config.setActiveModel).toHaveBeenCalledWith('gemini-pro');
143147
});
144148

145149
it('switches to backup model and updates config if requested is unavailable', () => {
146150
const config = createExtendedMockConfig();
151+
mockModelConfigService.getResolvedConfig
152+
.mockReturnValueOnce({
153+
model: 'gemini-pro',
154+
generateContentConfig: { temperature: 0.9, topP: 1 },
155+
})
156+
.mockReturnValueOnce({
157+
model: 'gemini-flash',
158+
generateContentConfig: { temperature: 0.1, topP: 1 },
159+
});
147160
mockAvailabilityService.selectFirstAvailable.mockReturnValue({
148161
selectedModel: 'gemini-flash',
149162
});
150-
mockModelConfigService.getResolvedConfig.mockReturnValue({
151-
generateContentConfig: { temperature: 0.1 },
152-
});
153163

154-
const currentConfig = { temperature: 0.9, topP: 1 };
155-
const result = applyModelSelection(config, 'gemini-pro', currentConfig);
164+
const result = applyModelSelection(config, { model: 'gemini-pro' });
156165

157166
expect(result.model).toBe('gemini-flash');
158167
expect(result.config).toEqual({
159168
temperature: 0.1,
160169
topP: 1,
161170
});
162171

172+
expect(mockModelConfigService.getResolvedConfig).toHaveBeenCalledWith({
173+
model: 'gemini-pro',
174+
});
163175
expect(mockModelConfigService.getResolvedConfig).toHaveBeenCalledWith({
164176
model: 'gemini-flash',
165177
});
@@ -168,12 +180,16 @@ describe('policyHelpers', () => {
168180

169181
it('consumes sticky attempt if indicated', () => {
170182
const config = createExtendedMockConfig();
183+
mockModelConfigService.getResolvedConfig.mockReturnValue({
184+
model: 'gemini-pro',
185+
generateContentConfig: {},
186+
});
171187
mockAvailabilityService.selectFirstAvailable.mockReturnValue({
172188
selectedModel: 'gemini-pro',
173189
attempts: 1,
174190
});
175191

176-
const result = applyModelSelection(config, 'gemini-pro');
192+
const result = applyModelSelection(config, { model: 'gemini-pro' });
177193
expect(mockAvailabilityService.consumeStickyAttempt).toHaveBeenCalledWith(
178194
'gemini-pro',
179195
);
@@ -182,16 +198,18 @@ describe('policyHelpers', () => {
182198

183199
it('does not consume sticky attempt if consumeAttempt is false', () => {
184200
const config = createExtendedMockConfig();
201+
mockModelConfigService.getResolvedConfig.mockReturnValue({
202+
model: 'gemini-pro',
203+
generateContentConfig: {},
204+
});
185205
mockAvailabilityService.selectFirstAvailable.mockReturnValue({
186206
selectedModel: 'gemini-pro',
187207
attempts: 1,
188208
});
189209

190210
const result = applyModelSelection(
191211
config,
192-
'gemini-pro',
193-
undefined,
194-
undefined,
212+
{ model: 'gemini-pro' },
195213
{
196214
consumeAttempt: false,
197215
},

packages/core/src/availability/policyHelpers.ts

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
resolveModel,
2626
} from '../config/models.js';
2727
import type { ModelSelectionResult } from './modelAvailabilityService.js';
28+
import type { ModelConfigKey } from '../services/modelConfigService.js';
2829

2930
/**
3031
* Resolves the active policy chain for the given config, ensuring the
@@ -155,31 +156,26 @@ export function selectModelForAvailability(
155156
*/
156157
export function applyModelSelection(
157158
config: Config,
158-
requestedModel: string,
159-
currentConfig?: GenerateContentConfig,
160-
overrideScope?: string,
159+
modelConfigKey: ModelConfigKey,
161160
options: { consumeAttempt?: boolean } = {},
162-
): { model: string; config?: GenerateContentConfig; maxAttempts?: number } {
163-
const selection = selectModelForAvailability(config, requestedModel);
161+
): { model: string; config: GenerateContentConfig; maxAttempts?: number } {
162+
const resolved = config.modelConfigService.getResolvedConfig(modelConfigKey);
163+
const model = resolved.model;
164+
const selection = selectModelForAvailability(config, model);
164165

165-
if (!selection?.selectedModel) {
166-
return { model: requestedModel, config: currentConfig };
166+
if (!selection) {
167+
return { model, config: resolved.generateContentConfig };
167168
}
168169

169-
const finalModel = selection.selectedModel;
170-
let finalConfig = currentConfig;
171-
172-
// If model changed, re-resolve config
173-
if (finalModel !== requestedModel) {
174-
const { generateContentConfig } =
175-
config.modelConfigService.getResolvedConfig({
176-
overrideScope,
177-
model: finalModel,
178-
});
170+
const finalModel = selection.selectedModel ?? model;
171+
let generateContentConfig = resolved.generateContentConfig;
179172

180-
finalConfig = currentConfig
181-
? { ...currentConfig, ...generateContentConfig }
182-
: generateContentConfig;
173+
if (finalModel !== model) {
174+
const fallbackResolved = config.modelConfigService.getResolvedConfig({
175+
...modelConfigKey,
176+
model: finalModel,
177+
});
178+
generateContentConfig = fallbackResolved.generateContentConfig;
183179
}
184180

185181
config.setActiveModel(finalModel);
@@ -190,7 +186,7 @@ export function applyModelSelection(
190186

191187
return {
192188
model: finalModel,
193-
config: finalConfig,
189+
config: generateContentConfig,
194190
maxAttempts: selection.attempts,
195191
};
196192
}

packages/core/src/core/baseLlmClient.test.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -776,13 +776,15 @@ describe('BaseLlmClient', () => {
776776
const getResolvedConfigMock = vi.mocked(
777777
mockConfig.modelConfigService.getResolvedConfig,
778778
);
779-
getResolvedConfigMock
780-
.mockReturnValueOnce(
781-
makeResolvedModelConfig(firstModel, { temperature: 0.1 }),
782-
)
783-
.mockReturnValueOnce(
784-
makeResolvedModelConfig(fallbackModel, { temperature: 0.9 }),
785-
);
779+
getResolvedConfigMock.mockImplementation((key) => {
780+
if (key.model === firstModel) {
781+
return makeResolvedModelConfig(firstModel, { temperature: 0.1 });
782+
}
783+
if (key.model === fallbackModel) {
784+
return makeResolvedModelConfig(fallbackModel, { temperature: 0.9 });
785+
}
786+
return makeResolvedModelConfig(key.model);
787+
});
786788

787789
// Availability selects the first model initially
788790
vi.mocked(mockAvailabilityService.selectFirstAvailable).mockReturnValue({

packages/core/src/core/baseLlmClient.ts

Lines changed: 63 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
EmbedContentParameters,
1111
GenerateContentResponse,
1212
GenerateContentParameters,
13+
GenerateContentConfig,
1314
} from '@google/genai';
1415
import type { Config } from '../config/config.js';
1516
import type { ContentGenerator } from './contentGenerator.js';
@@ -81,6 +82,19 @@ export interface GenerateContentOptions {
8182
maxAttempts?: number;
8283
}
8384

85+
interface _CommonGenerateOptions {
86+
modelConfigKey: ModelConfigKey;
87+
contents: Content[];
88+
systemInstruction?: string | Part | Part[] | Content;
89+
abortSignal: AbortSignal;
90+
promptId: string;
91+
maxAttempts?: number;
92+
additionalProperties?: {
93+
responseJsonSchema: Record<string, unknown>;
94+
responseMimeType: string;
95+
};
96+
}
97+
8498
/**
8599
* A client dedicated to stateless, utility-focused LLM calls.
86100
*/
@@ -104,7 +118,7 @@ export class BaseLlmClient {
104118
maxAttempts,
105119
} = options;
106120

107-
const { model, generateContentConfig } =
121+
const { model } =
108122
this.config.modelConfigService.getResolvedConfig(modelConfigKey);
109123

110124
const shouldRetryOnContent = (response: GenerateContentResponse) => {
@@ -123,18 +137,17 @@ export class BaseLlmClient {
123137

124138
const result = await this._generateWithRetry(
125139
{
126-
model,
140+
modelConfigKey,
127141
contents,
128-
config: {
129-
...generateContentConfig,
130-
...(systemInstruction && { systemInstruction }),
142+
abortSignal,
143+
promptId,
144+
maxAttempts,
145+
systemInstruction,
146+
additionalProperties: {
131147
responseJsonSchema: schema,
132148
responseMimeType: 'application/json',
133-
abortSignal,
134149
},
135150
},
136-
promptId,
137-
maxAttempts,
138151
shouldRetryOnContent,
139152
'generateJson',
140153
);
@@ -205,80 +218,81 @@ export class BaseLlmClient {
205218
maxAttempts,
206219
} = options;
207220

208-
const { model, generateContentConfig } =
209-
this.config.modelConfigService.getResolvedConfig(modelConfigKey);
210-
211221
const shouldRetryOnContent = (response: GenerateContentResponse) => {
212222
const text = getResponseText(response)?.trim();
213223
return !text; // Retry on empty response
214224
};
215225

216226
return this._generateWithRetry(
217227
{
218-
model,
228+
modelConfigKey,
219229
contents,
220-
config: {
221-
...generateContentConfig,
222-
...(systemInstruction && { systemInstruction }),
223-
abortSignal,
224-
},
230+
systemInstruction,
231+
abortSignal,
232+
promptId,
233+
maxAttempts,
225234
},
226-
promptId,
227-
maxAttempts,
228235
shouldRetryOnContent,
229236
'generateContent',
230237
);
231238
}
232239

233240
private async _generateWithRetry(
234-
requestParams: GenerateContentParameters,
235-
promptId: string,
236-
maxAttempts: number | undefined,
241+
options: _CommonGenerateOptions,
237242
shouldRetryOnContent: (response: GenerateContentResponse) => boolean,
238243
errorContext: 'generateJson' | 'generateContent',
239244
): Promise<GenerateContentResponse> {
240-
const abortSignal = requestParams.config?.abortSignal;
241-
242-
// Define callback to fetch context dynamically since active model may get updated during retry loop
243-
const getAvailabilityContext = createAvailabilityContextProvider(
244-
this.config,
245-
() => requestParams.model,
246-
);
245+
const {
246+
modelConfigKey,
247+
contents,
248+
systemInstruction,
249+
abortSignal,
250+
promptId,
251+
maxAttempts,
252+
additionalProperties,
253+
} = options;
247254

248255
const {
249256
model,
250-
config: newConfig,
257+
config: generateContentConfig,
251258
maxAttempts: availabilityMaxAttempts,
252-
} = applyModelSelection(
259+
} = applyModelSelection(this.config, modelConfigKey);
260+
261+
let currentModel = model;
262+
let currentGenerateContentConfig = generateContentConfig;
263+
264+
// Define callback to fetch context dynamically since active model may get updated during retry loop
265+
const getAvailabilityContext = createAvailabilityContextProvider(
253266
this.config,
254-
requestParams.model,
255-
requestParams.config,
267+
() => currentModel,
256268
);
257-
requestParams.model = model;
258-
if (newConfig) {
259-
requestParams.config = newConfig;
260-
}
261-
if (abortSignal) {
262-
requestParams.config = { ...requestParams.config, abortSignal };
263-
}
264269

265270
try {
266271
const apiCall = () => {
267272
// Ensure we use the current active model
268273
// in case a fallback occurred in a previous attempt.
269274
const activeModel = this.config.getActiveModel();
270-
if (activeModel !== requestParams.model) {
271-
requestParams.model = activeModel;
275+
if (activeModel !== currentModel) {
276+
currentModel = activeModel;
272277
// Re-resolve config if model changed during retry
273278
const { generateContentConfig } =
274279
this.config.modelConfigService.getResolvedConfig({
280+
...modelConfigKey,
275281
model: activeModel,
276282
});
277-
requestParams.config = {
278-
...requestParams.config,
279-
...generateContentConfig,
280-
};
283+
currentGenerateContentConfig = generateContentConfig;
281284
}
285+
const finalConfig: GenerateContentConfig = {
286+
...currentGenerateContentConfig,
287+
...(systemInstruction && { systemInstruction }),
288+
...additionalProperties,
289+
abortSignal,
290+
};
291+
const requestParams: GenerateContentParameters = {
292+
model: currentModel,
293+
config: finalConfig,
294+
contents,
295+
};
282296
return this.contentGenerator.generateContent(requestParams, promptId);
283297
};
284298

@@ -289,7 +303,7 @@ export class BaseLlmClient {
289303
getAvailabilityContext,
290304
onPersistent429: this.config.isInteractive()
291305
? (authType, error) =>
292-
handleFallback(this.config, requestParams.model, authType, error)
306+
handleFallback(this.config, currentModel, authType, error)
293307
: undefined,
294308
authType:
295309
this.authType ?? this.config.getContentGeneratorConfig()?.authType,
@@ -307,14 +321,14 @@ export class BaseLlmClient {
307321
await reportError(
308322
error,
309323
`API returned invalid content after all retries.`,
310-
requestParams.contents as Content[],
324+
contents,
311325
`${errorContext}-invalid-content`,
312326
);
313327
} else {
314328
await reportError(
315329
error,
316330
`Error generating content via API.`,
317-
requestParams.contents as Content[],
331+
contents,
318332
`${errorContext}-api`,
319333
);
320334
}

0 commit comments

Comments
 (0)