Fix: Create OpenAI model instance in build_native_gemini_model#3
Open
railway-app[bot] wants to merge 1 commit into
Open
Fix: Create OpenAI model instance in build_native_gemini_model#3railway-app[bot] wants to merge 1 commit into
railway-app[bot] wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds proper OpenAIModel construction in build_native_gemini_model for openai:-prefixed specs and updates tests accordingly. Flow diagram for build_native_gemini_model OpenAIModel branchflowchart TD
A[normalized_spec] --> B[normalize_provider_model_spec]
B --> C[spec.lower]
C --> D{prefix}
D --> E[return GoogleModel
GeminiModel]:::nodeGoogleGla
D --> F[return GoogleModel
GeminiModel vertexai]:::nodeGoogleVertex
D --> G[return OpenAIModel
api_key OPENAI_API_KEY]:::nodeOpenAI
D --> H[return spec string]
classDef nodeGoogleGla fill:#e0f7fa,stroke:#006064
classDef nodeGoogleVertex fill:#e8f5e9,stroke:#1b5e20
classDef nodeOpenAI fill:#f3e5f5,stroke:#4a148c
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider centralizing OpenAI API key retrieval (similar to
_google_api_key) instead of callingos.getenvinline, so missing or misconfigured keys can be handled consistently and with clearer error messages. - The
build_native_gemini_modeldocstring and naming still suggest it only returns a Google/Gemini model or OpenAI string; updating them to reflect the newOpenAIModelreturn path would make the behavior clearer to callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing OpenAI API key retrieval (similar to `_google_api_key`) instead of calling `os.getenv` inline, so missing or misconfigured keys can be handled consistently and with clearer error messages.
- The `build_native_gemini_model` docstring and naming still suggest it only returns a Google/Gemini model or OpenAI string; updating them to reflect the new `OpenAIModel` return path would make the behavior clearer to callers.
## Individual Comments
### Comment 1
<location path="backend/src/services/llm.py" line_range="38-40" />
<code_context>
if low.startswith("google-vertex:"):
model_id = spec.split(":", 1)[1].strip()
return GeminiModel(model_id, provider=GoogleProvider(vertexai=True))
+ if low.startswith("openai:"):
+ model_id = spec.split(":", 1)[1].strip()
+ return OpenAIModel(model_id, api_key=os.getenv("OPENAI_API_KEY"))
return spec
</code_context>
<issue_to_address>
**issue:** Guard against empty or missing model id in the `openai:` spec
When `normalized_spec` is just `"openai:"` (or otherwise has nothing after the colon), `model_id` becomes an empty string and you end up constructing `OpenAIModel("")`, which may fail in a confusing way. Consider validating that `model_id` is non-empty and either returning `spec` unchanged or raising a clearer error for malformed specs.
</issue_to_address>
### Comment 2
<location path="backend/tests/test_llm.py" line_range="14-18" />
<code_context>
-def test_build_native_openai_returns_string() -> None:
- assert build_native_gemini_model("openai:gpt-4o-mini") == "openai:gpt-4o-mini"
+def test_build_native_openai_returns_openai_model(monkeypatch: pytest.MonkeyPatch) -> None:
+ monkeypatch.setenv("OPENAI_API_KEY", "test-openai-key")
+ m = build_native_gemini_model("openai:gpt-4o-mini")
+ assert isinstance(m, OpenAIModel)
+ assert m.model_name == "gpt-4o-mini"
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert the OpenAIModel api_key to verify that OPENAI_API_KEY wiring works as expected.
The current test only checks that we build an OpenAIModel with the expected model_name. Please also assert that `m.api_key == "test-openai-key"` (or equivalent) so the test verifies the OPENAI_API_KEY env var is correctly wired into the model.
```suggestion
def test_build_native_openai_returns_openai_model(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setenv("OPENAI_API_KEY", "test-openai-key")
m = build_native_gemini_model("openai:gpt-4o-mini")
assert isinstance(m, OpenAIModel)
assert m.model_name == "gpt-4o-mini"
assert m.api_key == "test-openai-key"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+38
to
+40
| if low.startswith("openai:"): | ||
| model_id = spec.split(":", 1)[1].strip() | ||
| return OpenAIModel(model_id, api_key=os.getenv("OPENAI_API_KEY")) |
There was a problem hiding this comment.
issue: Guard against empty or missing model id in the openai: spec
When normalized_spec is just "openai:" (or otherwise has nothing after the colon), model_id becomes an empty string and you end up constructing OpenAIModel(""), which may fail in a confusing way. Consider validating that model_id is non-empty and either returning spec unchanged or raising a clearer error for malformed specs.
Comment on lines
+14
to
+18
| def test_build_native_openai_returns_openai_model(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setenv("OPENAI_API_KEY", "test-openai-key") | ||
| m = build_native_gemini_model("openai:gpt-4o-mini") | ||
| assert isinstance(m, OpenAIModel) | ||
| assert m.model_name == "gpt-4o-mini" |
There was a problem hiding this comment.
suggestion (testing): Also assert the OpenAIModel api_key to verify that OPENAI_API_KEY wiring works as expected.
The current test only checks that we build an OpenAIModel with the expected model_name. Please also assert that m.api_key == "test-openai-key" (or equivalent) so the test verifies the OPENAI_API_KEY env var is correctly wired into the model.
Suggested change
| def test_build_native_openai_returns_openai_model(monkeypatch: pytest.MonkeyPatch) -> None: | |
| monkeypatch.setenv("OPENAI_API_KEY", "test-openai-key") | |
| m = build_native_gemini_model("openai:gpt-4o-mini") | |
| assert isinstance(m, OpenAIModel) | |
| assert m.model_name == "gpt-4o-mini" | |
| def test_build_native_openai_returns_openai_model(monkeypatch: pytest.MonkeyPatch) -> None: | |
| monkeypatch.setenv("OPENAI_API_KEY", "test-openai-key") | |
| m = build_native_gemini_model("openai:gpt-4o-mini") | |
| assert isinstance(m, OpenAIModel) | |
| assert m.model_name == "gpt-4o-mini" | |
| assert m.api_key == "test-openai-key" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
build_native_gemini_model()receives anopenai:-prefixed spec (e.g.openai:gpt-4o-mini), it falls through all branches and returns the raw string. PydanticAI requires aModelobject, so passing a plain string causes 404 errors at inference time. The existing test also asserted the broken string-return behaviour, causing it to fail once the fix is applied.Solution
Added
from pydantic_ai.models.openai import OpenAIModeland a newopenai:branch inbuild_native_gemini_model()that extracts the model ID and constructs anOpenAIModelinstance usingOPENAI_API_KEY. The return type annotation was updated tostr | GoogleModel | OpenAIModel. The corresponding test was updated to assert anOpenAIModelinstance is returned and that itsmodel_namematches the spec, replacing the now-incorrect string-equality assertion.Changes
backend/src/services/llm.pybackend/tests/test_llm.pyGenerated by Railway
Summary by Sourcery
Ensure build_native_gemini_model returns proper model instances for OpenAI-prefixed specs instead of raw strings.
Bug Fixes:
Tests: