Make that vertexai and gemini translate to google for api keys file #323
Conversation
WalkthroughThis update introduces a static method Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Request
participant CE as Completion Method
participant MP as map_provider
participant GS as Google Credential Setup
U->>CE: Call completion(provider)
CE->>MP: Call map_provider(provider)
MP-->>CE: Return mapped_provider
alt mapped_provider == "google"
CE->>GS: Retrieve API settings and set GOOGLE_APPLICATION_CREDENTIALS
end
CE-->>U: Return completion result
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.8.2)edenai_apis/llmengine/llm_engine.py756-756: Use Remove (SIM118) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
edenai_apis/llmengine/llm_engine.py(4 hunks)edenai_apis/llmengine/tests/conftest.py(4 hunks)edenai_apis/llmengine/tests/test_llm_engine.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
edenai_apis/llmengine/llm_engine.py
755-755: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (5)
edenai_apis/llmengine/tests/test_llm_engine.py (1)
227-230: Test implementation looks good.The unit test for the new
map_providermethod is well structured and uses the fixture data effectively to verify mapping behavior across different provider names.edenai_apis/llmengine/tests/conftest.py (2)
1-1: Type annotations correctly updated.The import statement is properly extended to include the required types for the new fixture.
170-196: Comprehensive test data for provider mapping.The test data covers a wide range of provider mappings, including edge cases like empty strings and None values.
edenai_apis/llmengine/llm_engine.py (2)
65-65: Import addition looks good.The
remodule is required for the regex matching in the newmap_providermethod.
802-817: The Google credentials handling looks good.Setting the
GOOGLE_APPLICATION_CREDENTIALSenvironment variable for Google provider is correct and aligned with the PR objectives to make vertexai and gemini translate to Google.
| @@ -730,6 +731,8 @@ def image_generation( | |||
|
|
|||
| class StdLLMEngine(LLMEngine): | |||
|
|
|||
| PROVIDER_MAPPING = {"vertex_ai": "google", "gemini": "google"} | |||
There was a problem hiding this comment.
Provider mapping dictionary is inconsistent with test data.
The PROVIDER_MAPPING dictionary only includes "vertex_ai" and "gemini" keys, but the test fixture includes more specific patterns like "vertex_ai-text-models". This mismatch could lead to test failures.
Consider updating the mapping dictionary to support all patterns:
- PROVIDER_MAPPING = {"vertex_ai": "google", "gemini": "google"}
+ PROVIDER_MAPPING = {"vertex_ai.*": "google", "gemini": "google"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PROVIDER_MAPPING = {"vertex_ai": "google", "gemini": "google"} | |
| PROVIDER_MAPPING = {"vertex_ai.*": "google", "gemini": "google"} |
| @staticmethod | ||
| def map_provider(provider_name: str) -> str: | ||
| if provider_name is None: | ||
| return None | ||
| # Try to regex match the keys of PROVIDER_MAPPING and provider_name. The first one ot match wins | ||
| for key in StdLLMEngine.PROVIDER_MAPPING.keys(): | ||
| if re.match(key, provider_name, re.RegexFlag.IGNORECASE): | ||
| return StdLLMEngine.PROVIDER_MAPPING[key] | ||
| return provider_name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the regex pattern matching implementation.
The current implementation has two issues:
- It iterates through keys using
.keys()which is unnecessary - The regex pattern needs to match the full string, not just a part of it
@staticmethod
def map_provider(provider_name: str) -> str:
if provider_name is None:
return None
- # Try to regex match the keys of PROVIDER_MAPPING and provider_name. The first one ot match wins
- for key in StdLLMEngine.PROVIDER_MAPPING.keys():
- if re.match(key, provider_name, re.RegexFlag.IGNORECASE):
+ # Try to regex match the keys of PROVIDER_MAPPING and provider_name. The first one to match wins
+ for key in StdLLMEngine.PROVIDER_MAPPING:
+ if re.fullmatch(f"{key}.*", provider_name, re.IGNORECASE):
return StdLLMEngine.PROVIDER_MAPPING[key]
return provider_name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def map_provider(provider_name: str) -> str: | |
| if provider_name is None: | |
| return None | |
| # Try to regex match the keys of PROVIDER_MAPPING and provider_name. The first one ot match wins | |
| for key in StdLLMEngine.PROVIDER_MAPPING.keys(): | |
| if re.match(key, provider_name, re.RegexFlag.IGNORECASE): | |
| return StdLLMEngine.PROVIDER_MAPPING[key] | |
| return provider_name | |
| @staticmethod | |
| def map_provider(provider_name: str) -> str: | |
| if provider_name is None: | |
| return None | |
| # Try to regex match the keys of PROVIDER_MAPPING and provider_name. The first one to match wins | |
| for key in StdLLMEngine.PROVIDER_MAPPING: | |
| if re.fullmatch(f"{key}.*", provider_name, re.IGNORECASE): | |
| return StdLLMEngine.PROVIDER_MAPPING[key] | |
| return provider_name |
🧰 Tools
🪛 Ruff (0.8.2)
755-755: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
Summary by CodeRabbit
New Features
Tests