SD2 1334 add deepseek with deepseek api #348
Conversation
WalkthroughThis update removes several debugging print statements from multiple API modules and helper functions. In addition, the Deepseek API now employs a dynamic provider name and simplifies model parameter formatting. The changes also include the complete removal of the model registration function in the Litellm client and the deletion of a configuration file containing model parameters. No alterations were made to public entity declarations across these modifications. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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: 1
🧹 Nitpick comments (4)
edenai_apis/apis/deepseek/deepseek_api.py (2)
50-50: Simplified model parameter but f-string is redundant.The model parameter formatting has been simplified, which aligns with the PR objectives. However, the f-string formatting
f"{model}"is redundant since it's equivalent to just usingmodel.- model=f"{model}", + model=model,
104-104: Same redundant f-string formatting.Similarly to the previous comment, the f-string formatting is unnecessary here and could be simplified.
- model=f"{model}", + model=model,edenai_apis/tests/outputs.py (2)
15-17: Duplicate Import Warning: Remove Redundancy
The modulevalidate_all_provider_constraintsis imported twice—once at lines 6–8 and again at lines 15–17. This duplication can be removed for clarity and maintainability.Proposed change:
-from edenai_apis.utils.constraints import ( - validate_all_provider_constraints, -)🧰 Tools
🪛 Ruff (0.8.2)
16-16: Redefinition of unused
validate_all_provider_constraintsfrom line 7Remove definition:
validate_all_provider_constraints(F811)
28-43: Refinement Suggestion: Prefer Logging over Print in Test Helper
Within thefake_cron_checkfunction (especially at line 36), consider using a logging mechanism (e.g., Python'sloggingmodule) rather than
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
edenai_apis/apis/amazon/amazon_audio_api.py(0 hunks)edenai_apis/apis/api4ai/api4ai_api.py(0 hunks)edenai_apis/apis/base64/base64_api.py(0 hunks)edenai_apis/apis/deepseek/deepseek_api.py(3 hunks)edenai_apis/apis/google/google_translation_api.py(0 hunks)edenai_apis/apis/microsoft/microsoft_image_api.py(0 hunks)edenai_apis/apis/nyckel/nyckel_api.py(0 hunks)edenai_apis/apis/openai/openai_api.py(1 hunks)edenai_apis/apis/twelvelabs/helpers.py(0 hunks)edenai_apis/llmengine/clients/litellm_client/__init__.py(0 hunks)edenai_apis/llmengine/clients/llm_models/models.json(0 hunks)edenai_apis/tests/outputs.py(1 hunks)
💤 Files with no reviewable changes (9)
- edenai_apis/apis/base64/base64_api.py
- edenai_apis/apis/microsoft/microsoft_image_api.py
- edenai_apis/apis/api4ai/api4ai_api.py
- edenai_apis/apis/amazon/amazon_audio_api.py
- edenai_apis/apis/nyckel/nyckel_api.py
- edenai_apis/apis/google/google_translation_api.py
- edenai_apis/llmengine/clients/litellm_client/init.py
- edenai_apis/apis/twelvelabs/helpers.py
- edenai_apis/llmengine/clients/llm_models/models.json
🧰 Additional context used
🧬 Code Definitions (1)
edenai_apis/apis/deepseek/deepseek_api.py (1)
edenai_apis/llmengine/llm_engine.py (1)
LLMEngine(68-851)
🔇 Additional comments (4)
edenai_apis/apis/deepseek/deepseek_api.py (2)
23-23: Good use of class attribute for provider name.This change removes hardcoded provider name reference and uses the class attribute
self.provider_nameinstead, which improves maintainability and consistency throughout the code.
27-27: Good consistency with provider naming.Similar to the change above, using
self.provider_namehere ensures consistency between how the provider is referenced when loading settings and initializing the LLM client.edenai_apis/tests/outputs.py (1)
61-62: Good Cleanup: Removal of Debug Print forargs
The removal of the debug print statement that previously printed theargsvariable after validation is a positive change, cleaning up the test output.edenai_apis/apis/openai/openai_api.py (1)
34-34: Removal of Debug Print StatementThe removal of the debugging print (likely printing
self.api_settings) is a positive change because it prevents sensitive configuration details from being logged in production. If logging remains necessary, consider leveraging a proper logging framework configured with the appropriate log levels.
| self.api_key = self.api_settings.get("api_key") | ||
| self.headers = {"Authorization": f"Bearer {self.api_key}"} | ||
| self.api_key = self.api_settings["api_key"] |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Duplicate API Key Assignment
The code assigns self.api_key twice—first using the .get() method (line 35) and then directly via indexing (line 37). Additionally, self.headers is set on line 36 but is subsequently overridden between lines 41 and 45. This redundancy may lead to confusion and could mask potential misconfigurations if the key is missing. Consider consolidating the API key extraction and header assignment into a single, clear block with proper error handling. For instance, you might replace the duplicated assignments with:
- self.api_key = self.api_settings.get("api_key")
- self.headers = {"Authorization": f"Bearer {self.api_key}"}
- self.api_key = self.api_settings["api_key"]
+ self.api_key = self.api_settings.get("api_key")
+ if self.api_key is None:
+ raise ValueError("API key is missing in the settings.")
+ openai.api_key = self.api_keyThen, you can proceed with constructing the definitive header at line 41.
Duplicate API Key Assignment Detected
- The constructor in
edenai_apis/apis/openai/openai_api.pyassigns the API key twice:- First, via
.get("api_key")on line 35, which may returnNoneif the key is missing. - Then, directly through indexing on line 37, which overwrites the initial assignment.
- First, via
- Additionally, the temporary header construction on line 36 is redundant since a definitive header is built later (lines 41–45).
To improve clarity and prevent potential misconfigurations, consolidate the API key extraction with proper error handling. For example:
- self.api_key = self.api_settings.get("api_key")
- self.headers = {"Authorization": f"Bearer {self.api_key}"}
- self.api_key = self.api_settings["api_key"]
+ self.api_key = self.api_settings.get("api_key")
+ if self.api_key is None:
+ raise ValueError("API key is missing in the settings.")
+ openai.api_key = self.api_keyAfter this refactoring, proceed with constructing the definitive headers at line 41.
📝 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.
| self.api_key = self.api_settings.get("api_key") | |
| self.headers = {"Authorization": f"Bearer {self.api_key}"} | |
| self.api_key = self.api_settings["api_key"] | |
| self.api_key = self.api_settings.get("api_key") | |
| if self.api_key is None: | |
| raise ValueError("API key is missing in the settings.") | |
| openai.api_key = self.api_key |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
47-48: Ensure Dependency Lock ConsistencyRemoving the existing
poetry.lockfile and regenerating it withpoetry lockensures that the build uses the latest dependency specifications. However, please verify that this behavior aligns with your project's goals for reproducible builds. Regenerating the lock file on every CI run might lead to non-deterministic dependency versions over time. Consider caching strategies or conditional regeneration if strict reproducibility is required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
edenai_apis/apis/picsart/picsart_api.py (1)
30-31: Type signature improvements, but consider implementation alignment.The parameter type changes from
Optional[str] = Noneto more specific types (strforfileandstr = ""forfile_url) are good improvements that better express the API's requirements. However, there's a potential inconsistency:
The type hint for
filenow indicates it's required (non-optional), but the implementation at lines 49-55 still checksif file and not file_url, which suggestsfilecould be falsy.Consider either:
- Adding validation at the beginning to ensure
fileis provided if it's truly required- Or updating the implementation to match the type signature's intent
def image__background_removal( self, file: str, file_url: str = "", provider_params: Optional[Dict[str, Any]] = None, **kwargs, ) -> ResponseType[BackgroundRemovalDataClass]: + # Validate required parameter if file is truly required + if not file and not file_url: + raise ProviderException("No file or file_url provided") + """ Calls the Picsart Remove Background API.
Summary by CodeRabbit