fix: use service account file for vertexai client initialization#308
fix: use service account file for vertexai client initialization#308Alameyo wants to merge 2 commits intovitali87:mainfrom
Conversation
Summary of ChangesHello @Alameyo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue preventing Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with VertexAI client initialization by ensuring it uses a service account file for authentication instead of an API key. The changes involve skipping API key validation for VertexAI and providing the necessary vertexai and credentials parameters during client setup. The implementation is sound, and I have one minor suggestion to improve the robustness of the configuration validation.
codebase_rag/config.py
Outdated
| if self.provider_type == cs.GoogleProviderType.VERTEX: | ||
| return |
There was a problem hiding this comment.
For improved clarity and to prevent potential issues from misconfiguration, it's a good practice to explicitly check that the provider is Google before checking for the Vertex provider type. This ensures that API key validation is only skipped when both conditions are met, making the logic more robust.
| if self.provider_type == cs.GoogleProviderType.VERTEX: | |
| return | |
| if self.provider.lower() == cs.Provider.GOOGLE and self.provider_type == cs.GoogleProviderType.VERTEX: | |
| return |
Greptile SummaryEnables VertexAI support by skipping Key Changes:
Note: The credentials handling in Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Application
participant Config as ModelConfig
participant Analyzer as DocumentAnalyzer
participant Client as genai.Client
participant Vertex as VertexAI
App->>Config: validate_api_key()
alt VertexAI Provider
Config->>Config: Check provider_type == VERTEX
Config-->>App: Skip validation
else Other Providers
Config->>Config: Validate auth token exists
Config-->>App: Raise error if missing
end
App->>Analyzer: __init__(project_root)
Analyzer->>Config: Get active_orchestrator_config
alt VertexAI Provider
Analyzer->>Client: genai.Client(vertexai=True, credentials, project, location)
Client->>Vertex: Initialize with service account
Vertex-->>Client: Authenticated client
else GLA Provider
Analyzer->>Client: genai.Client(auth_token)
Client-->>Analyzer: Authenticated client
end
Analyzer-->>App: Ready with configured client
Last reviewed commit: 10ca677 |
| if orchestrator_config.provider_type == cs.GoogleProviderType.VERTEX: | ||
| self.client = genai.Client( | ||
| vertexai=True, | ||
| credentials=orchestrator_config.service_account_file, |
There was a problem hiding this comment.
Inconsistent credentials handling with providers/base.py:74-81. The provider implementation converts the service account file path to a credentials object using service_account.Credentials.from_service_account_file(), but this passes the file path string directly. Consider aligning the approach for consistency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/tools/document_analyzer.py
Line: 39
Comment:
Inconsistent credentials handling with `providers/base.py:74-81`. The provider implementation converts the service account file path to a credentials object using `service_account.Credentials.from_service_account_file()`, but this passes the file path string directly. Consider aligning the approach for consistency.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Fixes issue #307.
There are 2 parts of this PR.
Without these I was unable to run cgr on VertexAI