-
Notifications
You must be signed in to change notification settings - Fork 0
fix(models): copy provider_model_id when merging Vertex API models with static config #790
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
base: main
Are you sure you want to change the base?
Conversation
…ic config When fetching models from the Vertex API and merging them with static configuration, the provider_model_id was not being copied from static config to the normalized model. This caused the database sync to fall back to using the canonical model_id instead of the correct provider model ID (e.g., 'gemini-3-flash-preview' for 'gemini-3-flash'). This fix ensures that when the Vertex API successfully returns models, the provider_model_id from static config is properly merged, enabling correct database lookups for chat completion request saves and model health metrics recording. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
|
||
| # If we have static config for this model, use its pricing | ||
| # If we have static config for this model, merge its metadata | ||
| if model_id in static_by_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.
Bug: Model merging fails due to a mismatch between provider model IDs from the API and canonical model IDs used in the static configuration lookup, causing duplicate models.
Severity: CRITICAL
🔍 Detailed Analysis
In fetch_models_from_google_vertex(), models fetched from the Vertex API are normalized using their provider model ID (e.g., "gemini-3-flash-preview"). However, the static model configuration, static_by_id, is indexed by canonical model IDs (e.g., "gemini-3-flash"). The subsequent check if model_id in static_by_id: fails because it attempts to look up the provider ID in the map of canonical IDs. This prevents the merge logic from executing, resulting in API-sourced models having incorrect IDs and missing critical metadata like provider_model_id. This also causes duplicate model entries in the database, as both the incomplete API model and the full static model are added.
💡 Suggested Fix
To fix this, the code needs to correctly map an API model to its corresponding static configuration entry before attempting to merge. This could be done by performing a reverse lookup from the provider model ID to the canonical ID, or by modifying _normalize_vertex_api_model to return the canonical ID and set provider_model_id separately.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/services/google_vertex_client.py#L1670
Potential issue: In `fetch_models_from_google_vertex()`, models fetched from the Vertex
API are normalized using their provider model ID (e.g., `"gemini-3-flash-preview"`).
However, the static model configuration, `static_by_id`, is indexed by canonical model
IDs (e.g., `"gemini-3-flash"`). The subsequent check `if model_id in static_by_id:`
fails because it attempts to look up the provider ID in the map of canonical IDs. This
prevents the merge logic from executing, resulting in API-sourced models having
incorrect IDs and missing critical metadata like `provider_model_id`. This also causes
duplicate model entries in the database, as both the incomplete API model and the full
static model are added.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8438933
Summary
Follow-up fix to PR #775. This addresses a bug identified by Sentry's code review after PR #775 was merged.
Issue
When fetching models from the Google Vertex API and merging them with static configuration, the
provider_model_idwas not being copied from static config. This caused the database sync to fall back to using the canonicalmodel_idinstead of the correct provider model ID.Root Cause
In
fetch_models_from_google_vertex(), when the Vertex API successfully returns models, the code merges them with static config for pricing/tags/name/description, but it was missing theprovider_model_idfield.Impact
When the Vertex API is available (which is the preferred path), models like
gemini-3-flashwould have their canonical ID stored instead of the correct provider model ID (gemini-3-flash-preview), causing database lookup failures for:Fix
Added code to copy
provider_model_idfrom static config when merging API models.Test Plan
tests/services/test_model_catalog_sync.pytests/services/test_google_vertex_client.pyRelated
🤖 Generated with Claude Code
Greptile Overview
Greptile Summary
Overview
This PR fixes a critical bug where
provider_model_idwas not being copied from static configuration when merging Vertex API models. This is a follow-up to PR #775, addressing an oversight identified by Sentry's code review.What Changed
Added 4 lines of code (lines 1676-1679) in
fetch_models_from_google_vertex()to copyprovider_model_idfrom static config when merging API-fetched models with static configuration.Why This Matters
For Gemini 3 models, the canonical model ID (e.g.,
gemini-3-flash) differs from the provider model ID used in API requests (e.g.,gemini-3-flash-preview). Without this fix:provider_model_id, causing it to fall back to canonical IDCode Flow
_get_static_model_config) creates models with correctprovider_model_id(added in PR fix(models): add provider_model_id support for Google Vertex model sync #775)_fetch_models_from_vertex_api) retrieves models from Google_normalize_vertex_api_model) converts API response (doesn't setprovider_model_id)provider_model_idif present in static configprovider_model_idfor lookupsTesting
The existing test suite validates this fix:
test_model_catalog_sync.py::TestGoogleVertexProviderModelIdhas comprehensive testsgemini-3-flash→gemini-3-flash-previewmappingImpact Assessment
✅ Fix is correct and complete
if static.get("provider_model_id"):is appropriateConfidence Score: 5/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Client as Model Sync Service participant FMGV as fetch_models_from_google_vertex() participant API as Vertex AI API participant Static as Static Config participant Normalize as _normalize_vertex_api_model() participant DB as Database Sync Client->>FMGV: Request models FMGV->>Static: Get static model config Static-->>FMGV: Returns static models with provider_model_id FMGV->>API: Fetch models from API alt API Available API-->>FMGV: Returns API models loop For each API model FMGV->>Normalize: Normalize API model Normalize-->>FMGV: Normalized model (no provider_model_id) alt Model exists in static config Note over FMGV: Merge static metadata FMGV->>FMGV: Copy pricing, tags, name, description FMGV->>FMGV: Copy provider_model_id (THIS FIX) Note over FMGV: Critical for Gemini 3 models<br/>where canonical ID differs<br/>from provider model ID end end FMGV->>FMGV: Add static models not in API else API Unavailable FMGV->>FMGV: Use static config only end FMGV-->>Client: Returns merged models Client->>DB: Sync models with provider_model_id Note over DB: Uses provider_model_id for lookups<br/>during chat completion saves