-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Progressive migration to newer Gemini models and naming conventions #5990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Progressive migration to newer Gemini models and naming conventions #5990
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @HahaBill Thank you for your work on this, I left some minor points but I'll go ahead and approve the PR.
Also should we add a sunset date or version on which remove the migrations?
Let me know what you think!
src/api/providers/gemini.ts
Outdated
| } | ||
|
|
||
| if (modelId.startsWith("gemini-1.5-pro-")) { | ||
| return geminiDefaultModelId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed an inconsistency in the model mapping logic between Gemini and Vertex handlers. Here, gemini-1.5-pro-* maps to geminiDefaultModelId, while in the Vertex handler it explicitly maps to gemini-2.0-flash-001.
While both ultimately resolve to the same model (since geminiDefaultModelId is gemini-2.0-flash-001), would it be clearer to use the explicit model ID in both places for consistency? This would make the mapping logic more transparent and easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was planning to do that but realised that geminiDefaultModelId is a type of GeminiModelId, whereas Vertex models are type of VertexModelId. So there are different types.
(1) But gemini-2.0-flash-001 is currently hardcoded in the Vertex legacy mapping. I can actually create export const vertexDefaultGeminiModelId: VertexModelId = "gemini-2.0-flash-001" so then it's easier to maintain.
(2) Or we can do geminiDefaultModelId to be type of VertexModelId and GeminiModelId.
Which one of the two options do you think is better? I saw there is a PR that tries to separate Vertex from Gemini so maybe option (1) sounds better?
| let id = modelId ? this.mapLegacyGeminiModel(modelId) : geminiDefaultModelId | ||
|
|
||
| if (modelId && modelId !== id) { | ||
| this.options.apiModelId = id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the decision to modify this.options.apiModelId directly here. Could this have unintended side effects if the options object is shared or referenced elsewhere?
Would it be safer to keep the original model ID unchanged and only use the mapped ID internally? This would preserve the original user configuration while still achieving the migration goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is because of doing an immediate automatic sync between the backend and frontend (webview-ui). This is why the mapping function on the backend and frontend is the same.
If users choose another models from the selection in the profile settings then the original model id would be overwritten by new (non-legacy) model id and that is fine. This if (modelId && modelId !== id) { will only happen if it's a legacy model.
We could just not do the mapping on the frontend-side but then users have to choose models for each Gemini profile. If not then the original (legacy) model ID might be still used (because that is still stored) and this could cause API errors because the original (legacy) model ID is not accessible by the Gemini API anymore.
I think the longer we keep the migration, the safer it is. Currently, it's summer so I expect people to not code that much :D I would say around mid-October sounds good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to avoid duplicating the logic into this file? At least it seems the same as what's above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is exactly the same, I will try to unify them into one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I implemented the changes, now the mapping functions should be in the gemini.ts and vertex.ts in types folder. I did the test procedures which you can find in this PR's description (in debug mode) and everything seems to work.
The thing I noticed now that in old Gemini profiles, the "Save" is not disabled and you can either discard the changes or click on "Save".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be because when I change the model in useSelectedModel.ts which is tracked by SettingsViews.tsx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with (preventing the dirty state): https://github.com/HahaBill/Roo-Code/blob/i/update-gemini-and-vertex-models/webview-ui/src/components/settings/SettingsView.tsx#L235-L245
| const handleLegacyGeminiModelMapping = useCallback( | ||
| <K extends keyof ProviderSettings>( | ||
| field: K, | ||
| previousValue: ProviderSettings[K], | ||
| value: ProviderSettings[K], | ||
| provider: string | undefined, | ||
| updatedApiConfiguration: ProviderSettings, | ||
| ): boolean => { | ||
| if (field !== "apiModelId" || typeof previousValue !== "string" || typeof value !== "string") { | ||
| return false | ||
| } | ||
|
|
||
| let isLegacyMapping = false | ||
| if (provider === "gemini") { | ||
| isLegacyMapping = mapLegacyGeminiModel(previousValue) === value | ||
| } else if (provider === "vertex") { | ||
| isLegacyMapping = mapLegacyVertexModel(previousValue) === value | ||
| } | ||
|
|
||
| if (isLegacyMapping && currentApiConfigName) { | ||
| vscode.postMessage({ | ||
| type: "upsertApiConfiguration", | ||
| text: currentApiConfigName, | ||
| apiConfiguration: updatedApiConfiguration, | ||
| }) | ||
| } | ||
|
|
||
| return isLegacyMapping | ||
| }, | ||
| [currentApiConfigName], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new change that I did, I realised that it wasn't persistently stored. We don't need the legacy mapping in gemini.ts anymore but we can leave it there just for the extra security.
Overall: we should have a clean ap state now with migrations being persistently stored with doing upsert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new change that I did, I realised that it wasn't persistently stored. We don't need the legacy mapping in
gemini.tsanymore but we can leave it there just for the extra security.Overall: we should have a clean app state now with migrations being persistently stored with doing upsert
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @HahaBill for addressing the reviews, this looks good
|
I'll be closing this as stale for now, however feel free to let me know if we should get this PR ready again. |

Related GitHub Issue
Closes: #5444
Description
Progressively migrating to new Gemini models and naming conventions. See in #5444.
Test Procedure
Frontend Testing
Backend Testing
GeminiHandler.getModel()this.options.apiModelIdmatches the frontend displayPre-Submission Checklist
Important
Update Gemini and Vertex models, add legacy model mapping, and enhance tests for model ID migration.
geminiModelsandvertexModelsingemini.tsandvertex.tsto include new models and remove outdated ones.legacyGeminiModelsandlegacyVertexModelsto handle legacy model IDs.mapLegacyGeminiModel()ingemini.tsanduseSelectedModel.tsto map legacy Gemini model IDs to current models.mapLegacyVertexModel()invertex.tsanduseSelectedModel.tsto map legacy Vertex model IDs to current models.gemini.spec.tsandvertex.spec.tsto verify legacy model ID mapping to current models.This description was created by
for 6dab2cd. You can customize this summary. It will automatically update as commits are pushed.