feat(ai): add Google Gemini LLM provider support#3968
feat(ai): add Google Gemini LLM provider support#3968rajnisk wants to merge 9 commits intoOWASP:feature/nestbot-ai-assistantfrom
Conversation
- Add GoogleEmbedder implementation for embeddings - Update LLM config to support provider selection (OpenAI/Google) - Add Django settings for Gemini (GOOGLE_API_KEY, GOOGLE_MODEL_NAME, LLM_PROVIDER) - Update embedding factory to support Google provider - Add tests for Google LLM provider configuration - Update pyproject.toml to include google-genai extra - Add genai and generativeai to custom dictionary This enables contributors to run NestBot locally using a free Google Gemini API key as an alternative to OpenAI. Resolves part of OWASP#3693
Summary by CodeRabbitRelease Notes
WalkthroughSwitches LLM/embedder configuration from environment-variable reads to Django settings, adds a Google GenAI embedder with SDK/REST fallbacks, updates dependency extras, adds LLM-related settings and .env example entries, and updates tests and spell-check dictionary for Google support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 files
Confidence score: 2/5
- High risk of runtime failure in
backend/apps/ai/embeddings/google.py: using deprecatedgoogle.generativeaiAPIs with the newgoogle.genaiSDK will raiseAttributeErrorwhen deployed. - Silent fallback to OpenAI in
backend/apps/ai/common/llm_config.pycan mask misconfiguration and change provider behavior without warning, which is user-impacting. - Pay close attention to
backend/apps/ai/embeddings/google.py,backend/apps/ai/common/llm_config.py,backend/tests/apps/ai/common/llm_config_test.py,backend/settings/base.py- SDK API mismatch, provider fallback behavior, test not covering defaults, and inconsistent env var naming.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/ai/embeddings/google.py">
<violation number="1" location="backend/apps/ai/embeddings/google.py:51">
P0: Bug: `genai.configure()` and `genai.embed_content()` are APIs from the deprecated `google.generativeai` package, not the new `google.genai` SDK imported on line 5. When the new SDK is installed, this will raise `AttributeError` at runtime.
The new `google.genai` SDK uses a `Client` object pattern:
```python
client = genai.Client(api_key=self.api_key)
result = client.models.embed_content(model=self.model, contents=text)
The genai.configure() + genai.embed_content() pattern only works with the old deprecated google.generativeai package. Since the primary import on line 5 is the new SDK, this code will fail in the happy path.
(Based on your team's feedback about maintaining consistent and accurate naming.) [FEEDBACK_USED]
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/.env.example`:
- Around line 24-25: The two new .env keys are out of alphabetical order; move
the lines for DJANGO_GOOGLE_API_KEY=None and DJANGO_LLM_PROVIDER=None into the
DJANGO_* block sorted alphabetically: place DJANGO_GOOGLE_API_KEY=None
immediately after DJANGO_ELEVENLABS_API_KEY and then place
DJANGO_LLM_PROVIDER=None directly after DJANGO_GOOGLE_API_KEY and before
DJANGO_OPEN_AI_SECRET_KEY so all DJANGO_* entries remain alphabetized.
In `@backend/apps/ai/embeddings/google.py`:
- Around line 109-119: The comment claiming "SDK handles batching automatically"
is inaccurate because the code in the self.use_sdk branch calls
genai.embed_content in a per-text loop over texts; update the code to either (A)
use the SDK's actual batch embedding API (replace the per-text loop calling
genai.embed_content with the SDK batch method and extract embeddings) or (B) if
no batch API is available, change the comment to accurately state that
embeddings are requested sequentially and keep the loop; locate the branch using
self.use_sdk, genai, genai.embed_content, model, and texts to implement the
chosen fix.
- Line 86: The code constructs the REST fallback URL with the API key embedded
(endpoint =
f"{self.base_url}/models/{self.model}:embedContent?key={self.api_key}"), which
exposes credentials in logs; change the request to omit the key from the URL and
instead send it in an HTTP header (use 'x-goog-api-key': self.api_key) when
building the embedding request in the Google embeddings class/method that
constructs endpoint and performs the REST call, and also ensure any logging or
error handlers that might log the full URL redact query parameters (remove or
mask self.api_key) before logging.
- Around line 34-46: The constructor (__init__) currently ignores the passed
model and hardcodes self._dimensions = 768 which can lead to incorrect
get_dimensions() values; update __init__ in the Google embedder to derive
_dimensions from the requested model by either (a) adding a model→dimensions
mapping and setting self._dimensions = MODEL_DIMENSIONS.get(self.model) with a
sensible default, or (b) validating the provided self.model against supported
models and raising a clear error if it's unsupported (or forcing it to
"gemini-embedding-001"); ensure references to self.model and get_dimensions()
remain consistent and include a clear fallback/validation path so downstream
vector sizes are correct.
- Around line 5-23: The import logic is wrong because google.genai (new package)
lacks genai.configure() and genai.embed_content(); update the import and runtime
branching so the code supports both APIs: try importing google.generativeai
first (the deprecated package) and use its genai.configure() and
genai.embed_content() paths when available, otherwise import google.genai and
instantiate google.genai.Client and call the Client-based embedding methods
(e.g., Client embeddings/embedding creation calls) instead of
genai.configure()/genai.embed_content(); add a runtime feature check (hasattr or
try/except) around genai.configure and embed_content to choose the correct code
path and preserve existing behavior for both google.generativeai and
google.genai.
- Around line 75-97: The SDK call genai.embed_content returns an
EmbedContentResponse object (with an embeddings list), not a dict; update
embed_query (function embed_query) to read the vector from
result.embeddings[0].values instead of result["embedding"], and update
embed_documents (function embed_documents) to collect vectors with [e.values for
e in result.embeddings] (and perform the batching as a single SDK call instead
of iterating per-item). Ensure both code paths (use_sdk branch) use these
attribute accesses and keep the REST fallback unchanged.
🧹 Nitpick comments (3)
backend/apps/ai/common/llm_config.py (1)
36-47: Simplify the fallback guard —"openai"and"google"are unreachable here.Since both known providers already
returnon lines 23 and 29, theprovider not in ("openai", "google")check on line 37 is always true when reached. You can simplify to justif provider:.♻️ Suggested simplification
- if provider and provider not in ("openai", "google"): + if provider:backend/settings/base.py (1)
221-232: LGTM — good documentation on theValue()vsSecretValue()trade-off forGOOGLE_API_KEY.The comment clearly explains why
SecretValue()isn't used. One thing to note: whenLLM_PROVIDER=googlebutGOOGLE_API_KEYis unset, it will beNoneat runtime, which will surface as an opaque API error. Consider adding an early startup check (e.g., inAppConfig.ready()or inget_llm()) that raises a clearImproperlyConfigurederror. Based on learnings, the redundantenviron_nameremoval onOPEN_AI_SECRET_KEYis correct.backend/tests/apps/ai/common/llm_config_test.py (1)
54-78: Consider adding a test for unset/emptyLLM_PROVIDERto cover the silent fallback path.The current unsupported-provider test verifies warning + fallback, but there's no test for when
LLM_PROVIDERisNoneor""(the "not specified" path inllm_config.pythat falls back to OpenAI without logging a warning). This is a distinct code path worth covering.
- Fix import order: use google.generativeai first (has configure/embed_content) - Add support for new google.genai Client API with runtime feature detection - Fix return value access: use result.embeddings[0].values (object, not dict) - Make dimensions configurable: module-level MODEL_DIMENSIONS with validation - Update comments: accurate sequential processing descriptions - Fix linting errors: noqa comments, specific exceptions, refactored raise Addresses CodeRabbit review suggestions for better API compatibility and correct embedding response handling.
There was a problem hiding this comment.
4 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/ai/embeddings/google.py">
<violation number="1" location="backend/apps/ai/embeddings/google.py:117">
P1: Bug: `self.base_url` is never set when the new SDK path is selected, but the fallback to REST API (triggered when the SDK call fails) uses `self.base_url`, causing an `AttributeError`. Set `self.base_url` unconditionally in `__init__` so the REST fallback always works.</violation>
<violation number="2" location="backend/apps/ai/embeddings/google.py:157">
P1: Bug: Wrong result access pattern for the deprecated `google.generativeai` API. The deprecated `embed_content()` returns a **dictionary** with an `"embedding"` key, not an object with `.embeddings` attribute. The original code (`result["embedding"]`) was correct. This will crash at runtime with `AttributeError` when the deprecated SDK is used.</violation>
<violation number="3" location="backend/apps/ai/embeddings/google.py:165">
P1: Bug: Wrong parameter name for the new `google.genai` Client API. The official docs use `contents` (plural), not `content` (singular). This will likely raise a `TypeError` or silently fail.</violation>
<violation number="4" location="backend/apps/ai/embeddings/google.py:221">
P1: Bug: Same incorrect result access for deprecated API in `embed_documents`. Should use `result["embedding"]` instead of `result.embeddings[0].values`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@backend/apps/ai/embeddings/google.py`:
- Around line 159-176: The current parsing in the self.use_new_sdk branch of the
embed extraction (around client.models.embed_content and variable result) can
silently return an empty list; change it so that if no embedding is found from
any checked paths (result.embeddings[0].values, result.embedding.values, or
dict-like result.get("embedding")...), you raise a clear exception (e.g.,
ValueError) that includes contextual information (model name, input length, and
a safe repr(result) or result keys) instead of returning []; this makes callers
aware of malformed SDK responses and aids debugging.
- Around line 11-29: The deprecation warning for google.generativeai is emitted
at import time even when the Google provider isn't selected; move the
warnings.warn call out of the top-level import block and instead emit it inside
the embedder's initializer (e.g., the class __init__ where the embedder is
instantiated) only when use_deprecated_api is True and genai_deprecated was
chosen; keep the import/hasattr checks in the module to set
genai_deprecated/use_deprecated_api, but defer calling warnings.warn until the
embedder's __init__ so the warning appears only when the deprecated SDK is
actually used.
- Around line 112-127: Initialize self.base_url unconditionally in the class
constructor before the genai_client_module branching so it exists whether using
the new SDK, deprecated SDK, or REST fallback; specifically set self.base_url =
"https://generativelanguage.googleapis.com/v1beta" prior to the try/except where
you instantiate genai_client_module.Client (and keep the existing else fallback
unchanged), so embed_query and embed_documents can safely reference
self.base_url even if the SDK path later fails at runtime.
- Around line 269-281: The batch REST payload omits the per-item "model" field
required by Google's batchEmbedContents API; update the requests JSON
comprehension to include "model": self.model in each request object (the code
that builds json={"requests": [...]}) so each entry is {"model": self.model,
"content": {"parts": [{"text": text}]}}; keep using self.base_url, endpoint, and
self.api_key as before and ensure response handling
(response.raise_for_status(), response.json(), and returning embeddings) remains
unchanged.
- Around line 149-157: The deprecated google.generativeai.embed_content() call
in the code path using self.use_deprecated_sdk returns a dict (e.g.
{"embedding": [...]}) not an object with .embeddings, so change the embed_query
and embed_documents handling to read the embedding from the dict safely: when
genai.embed_content(...) returns, check for dict keys ("embedding" or
"embeddings") and extract the vector accordingly (e.g. result.get("embedding")
or result.get("embeddings")[0] if needed), while preserving the existing return
shape; update the branches in the methods that call genai.embed_content (the
code guarded by self.use_deprecated_sdk and using genai) to support both dict
and object shapes to avoid AttributeError.
🧹 Nitpick comments (1)
backend/apps/ai/embeddings/google.py (1)
224-267: Per-item REST fallback in the new SDK path defeats batching and duplicates logic.When the new SDK path fails for individual documents (lines 252-266), each item falls back to individual REST calls instead of collecting failures and using the batch endpoint. Additionally, the REST call logic and SDK response parsing are duplicated across
embed_queryandembed_documents.Consider extracting a
_embed_single_rest(text)helper and a_parse_sdk_result(result)helper to reduce duplication and centralize error handling.
- Fix import order: use google.generativeai first (has configure/embed_content) - Add support for new google.genai Client API with runtime feature detection - Fix return value access: use result.embeddings[0].values (object, not dict) - Make dimensions configurable: module-level MODEL_DIMENSIONS with validation - Update comments: accurate sequential processing descriptions - Fix linting errors: noqa comments, specific exceptions, helper function for raises - Defer deprecation warning to __init__ (only fires when embedder is used) - Initialize base_url early to prevent AttributeError in SDK fallback paths - Raise error instead of returning empty data for unrecognized API structures - Use x-goog-api-key header instead of query parameter for API key security - Add required model field to batchEmbedContents requests per API docs Addresses CodeRabbit review suggestions for better API compatibility, correct embedding response handling, improved security, and API compliance.
- Fix inconsistent naming: OPENAI_MODEL_NAME → OPEN_AI_MODEL_NAME - Fix test_get_llm_google to actually test default model - Change silent fallback to logger.error for better visibility - Verify SDK usage is correct (already handles both APIs)
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/ai/embeddings/google.py">
<violation number="1" location="backend/apps/ai/embeddings/google.py:310">
P1: Bug: The `model` field in the batch embed request body is missing the required `models/` prefix. According to the Google Generative AI API documentation, the model field in each `EmbedContentRequest` must use the format `models/{model}`. Without this prefix, the batch REST API call will fail.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/apps/ai/embeddings/google.py`:
- Around line 174-204: The try/except around the new SDK path currently catches
ValueError, which causes _raise_embedding_extraction_error() to be swallowed and
silently fall back to REST; update the handler so the helper's error is not
caught (either remove ValueError from the except tuple in the block surrounding
client.models.embed_content, or change _raise_embedding_extraction_error() to
raise a distinct exception type and let the except catch only
AttributeError/TypeError), and also remove or replace the placeholder comment
about the implementation; locate symbols use_new_sdk,
client.models.embed_content, and _raise_embedding_extraction_error to apply the
fix.
- Around line 298-320: The batchEmbedContents request body is setting "model":
self.model without the required "models/" prefix; update the list comprehension
that builds the request items (the dict with "model" in the REST fallback block
that posts to endpoint and uses batchEmbedContents) to set "model" to
f"models/{self.model}" (or equivalent string concatenation) so each item's model
matches the URL format; keep all other fields the same and ensure the generated
JSON keys still match the API schema.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
backend/settings/base.py (1)
221-232: Good cleanup onOPEN_AI_SECRET_KEYand well-documented rationale forGOOGLE_API_KEY.The removal of the redundant
environ_nameis correct. The comment explaining whyValue()is used instead ofSecretValue()forGOOGLE_API_KEYis helpful.One consideration: since
OPEN_AI_SECRET_KEYusesSecretValue(), it must be set in the environment at startup — even for Google-only deployments — becauseSecretValueraisesImproperlyConfiguredduring class resolution. This is consistent with the fallback-to-OpenAI design, but it means a pure Google-only setup isn't possible without also providing an OpenAI key (or a dummy value). If that's intentional, it might be worth a brief comment. If Google-only setups should be supported in the future,OPEN_AI_SECRET_KEYwould need the sameValue(default=None)treatment.Based on learnings, the redundant
environ_nameremoval onOPEN_AI_SECRET_KEYaligns with the established pattern that django-configurations automatically prefixes withDJANGO_and uses the setting name by default.backend/apps/ai/common/llm_config.py (2)
36-47: Simplify the redundant guard on line 37.At this point in the function,
provideris guaranteed to be neither"openai"nor"google"(both returned early). Theprovider not in ("openai", "google")check is alwaysTruehere. Simplify to justif provider:.♻️ Suggested simplification
# Fallback to OpenAI if provider not recognized or not specified - if provider and provider not in ("openai", "google"): + if provider: logger.error( "Unrecognized LLM_PROVIDER '%s'. Falling back to OpenAI. " "Supported providers: 'openai', 'google'", provider, )
22-47: Consider extracting the repeated OpenAI LLM construction.The OpenAI
LLM(...)call on lines 23–27 is duplicated in the fallback on lines 43–47. If this grows (e.g., adding more kwargs), you'd need to update both. A small helper or variable could reduce drift risk.♻️ One possible approach
def get_llm() -> LLM: - """Get configured LLM instance. - - Returns: - LLM: Configured LLM instance based on settings. - - """ + """Get configured LLM instance based on settings.""" provider = settings.LLM_PROVIDER + def _openai_llm() -> LLM: + return LLM( + model=settings.OPEN_AI_MODEL_NAME, + api_key=settings.OPEN_AI_SECRET_KEY, + temperature=0.1, + ) + if provider == "openai": - return LLM( - model=settings.OPEN_AI_MODEL_NAME, - api_key=settings.OPEN_AI_SECRET_KEY, - temperature=0.1, - ) + return _openai_llm() if provider == "google": return LLM( model=settings.GOOGLE_MODEL_NAME, base_url="https://generativelanguage.googleapis.com/v1beta/openai/", api_key=settings.GOOGLE_API_KEY, temperature=0.1, ) # Fallback to OpenAI if provider not recognized or not specified - if provider and provider not in ("openai", "google"): + if provider: logger.error( "Unrecognized LLM_PROVIDER '%s'. Falling back to OpenAI. " "Supported providers: 'openai', 'google'", provider, ) - return LLM( - model=settings.OPEN_AI_MODEL_NAME, - api_key=settings.OPEN_AI_SECRET_KEY, - temperature=0.1, - ) + return _openai_llm()backend/tests/apps/ai/common/llm_config_test.py (2)
54-78: Good coverage of the fallback path with logger verification.The decorator ordering and parameter mapping (
mock_llm,mock_logger) are correct. The test properly verifies both the error logging and the OpenAI fallback.One gap worth noting: there's no test for when
LLM_PROVIDERis empty/falsy (e.g.,""). In that case the code silently falls back to OpenAI without logging an error — a subtly different path from the unsupported-provider case. Consider adding a test for that edge case to lock down the behavior.
80-125: Good coverage for Google provider paths.Both default and custom model tests for the Google provider look correct. The expected
base_url,api_key, andtemperaturevalues match the implementation inllm_config.py.One edge case to consider: what happens when
DJANGO_LLM_PROVIDER=googlebutDJANGO_GOOGLE_API_KEYis not set? The setting defaults toNone, soget_llm()would passapi_key=NonetoLLM(...), which would likely fail at call time. A test covering this scenario could document whether that's an expected failure mode or whetherget_llm()should validate the key and fall back.
- Add NoReturn type hints to raise functions - Remove unreachable return statements Fixes type checker errors about missing return statements.
- Remove placeholder comment from new SDK implementation
- Fix new SDK parameter: use 'contents' (plural) instead of 'content'
- Verify ValueError handling is correct (not caught, propagates)
- Verify batch model format is correct (models/{model})
All CodeRabbit AI review issues resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/apps/ai/embeddings/google.py`:
- Around line 248-267: The code currently raises a ValueError via
_raise_embedding_extraction_error() which propagates out and prevents the
intended REST fallback; update the exception handling around the SDK extraction
block (the except that currently reads except (AttributeError, TypeError) as e)
to also catch ValueError (e.g., except (AttributeError, TypeError, ValueError)
as e) so that the code warns and continues to the REST fallback, or
alternatively change _raise_embedding_extraction_error() to emit a warning and
return/continue instead of raising so the REST fallback code after the
try/except is reached.
- Around line 110-118: The function _extract_embedding_from_result currently
assumes embedding values are dicts with a "values" key and will crash for
deprecated responses where "embedding" or "embeddings"[0] is a plain list;
update _extract_embedding_from_result to check the type of result["embedding"]
and result["embeddings"][0] and, if they are lists (or tuples), return them
directly instead of calling .get("values"), otherwise fall back to accessing the
"values" key or raising via
_raise_deprecated_api_dict_error/_raise_deprecated_api_error as appropriate so
both new and deprecated dict shapes are handled safely.
🧹 Nitpick comments (3)
backend/apps/ai/embeddings/google.py (3)
57-62: Return type should beNoReturn, not implicitNone.The other two helper functions (
_raise_deprecated_api_error,_raise_deprecated_api_dict_error) correctly use-> NoReturn. This one always raises but declares no return type, which means type checkers infer-> None.Fix
-def _raise_embedding_extraction_error() -> None: +def _raise_embedding_extraction_error() -> NoReturn:
234-235: Remove "placeholder" comment before merging.Line 235 says "this is a placeholder implementation" — this should be removed or replaced with accurate documentation, as it undermines confidence in the code path.
232-253: Duplicated new-SDK extraction logic — extract to a helper.The embedding extraction pattern (lines 244-253 and 322-333) is duplicated between
embed_queryandembed_documents. Consider extracting it to a helper function similar to_extract_embedding_from_resultfor the deprecated path, to keep the methods focused and reduce duplication.Also applies to: 310-333
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/apps/ai/embeddings/google.py (3)
57-62: Return type should beNoReturn, notNone.This function unconditionally raises. The other two helpers (
_raise_deprecated_api_error,_raise_deprecated_api_dict_error) correctly annotate-> NoReturn. This one should match for consistency and to help static analysis.Suggested fix
-def _raise_embedding_extraction_error() -> None: +def _raise_embedding_extraction_error() -> NoReturn:
282-357:embed_documentsmixes per-item SDK calls with per-item REST fallback — consider batch REST as the fallback.Currently, in the new SDK path (lines 306-357), if the SDK fails for one text, the code falls back to individual REST calls per item. However, the pure REST path (lines 359-381) already uses the batch
batchEmbedContentsendpoint, which is more efficient. If the SDK consistently fails (e.g., incompatible version), every item will trigger a separate REST call instead of a single batch request.Consider collecting failed items and batch-calling the REST endpoint once, rather than issuing per-item REST requests inside the loop.
337-337: Falsy checkif embedding_values:will treat a valid empty embedding[]as failure.While unlikely in practice (embeddings should always be non-empty), using
if embedding_values is not None:is semantically more correct and avoids a subtle bug if the API ever returns an empty list.Suggested fix
- if embedding_values: + if embedding_values is not None:
- Fix ValueError handling: catch ValueError to allow REST fallback - Fix embedding extraction: handle plain lists in addition to dicts with 'values' key - Ensure all SDK extraction failures gracefully fall back to REST API All CodeRabbit AI review issues resolved for LGTM approval.
- Remove _raise_embedding_extraction_error() calls - Let code fall through naturally to REST API when extraction fails - Remove unused function and constant - Remove ValueError from except clauses - Fix line length linting error Fixes CodeRabbit issue about ValueError being caught immediately.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/ai/embeddings/google.py">
<violation number="1" location="backend/apps/ai/embeddings/google.py:319">
P2: Inconsistent exception handling: `embed_query` warns on SDK failure, but `embed_documents` silently swallows the same exceptions with `pass`. Consider adding a warning here too, matching the pattern in `embed_query`, so developers can diagnose unexpected SDK behavior during batch operations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ling - Add warning to embed_documents when SDK fails, matching embed_query pattern - Ensures consistent exception handling across both methods - Makes it easier to diagnose SDK issues during batch operations Addresses CodeRabbit suggestion for consistent exception handling.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/apps/ai/embeddings/google.py (4)
239-242:result.get()on SDK response object is effectively dead code.The
google.genaiEmbedContentResponseis not a dict and has no.get()method. This line always raisesAttributeError, which is caught by line 244. It works as a fall-through to REST, but the "try to access as dict-like" comment is misleading. Consider replacing with an explicit fall-through:Suggested simplification
- # Fallback: try to access as dict-like - embedding = result.get("embedding", {}).get("values", []) - if embedding: - return embedding - # If we can't extract embedding, fall through to REST API + # If neither extraction method works, fall through to REST API
296-351: Per-item REST fallback in the new SDK loop is inefficient compared to the batch REST path.If the new SDK consistently fails (e.g., unexpected response structure), this loop makes N individual
embedContentREST calls (line 336), while the pure REST path below (line 355) uses a singlebatchEmbedContentscall. For large document sets, this is significantly slower.Consider detecting SDK failure on the first item and switching entirely to the batch REST endpoint for the remaining items:
Sketch
if self.use_new_sdk and self.client: results = [] + sdk_failed = False for text in texts: embedding_values = None + if sdk_failed: + break try: result = self.client.models.embed_content(...) # ... extraction logic ... except (AttributeError, TypeError) as e: - # warn and continue per-item REST fallback + sdk_failed = True + break - if embedding_values: - results.append(embedding_values) - else: - # individual REST call per item - ... + if embedding_values: + results.append(embedding_values) + if sdk_failed or len(results) < len(texts): + # Fall through to batch REST for remaining/all texts + remaining = texts[len(results):] + # Use batch REST endpoint for remaining texts + ... return results
282-294: No error handling in deprecated SDKembed_documentsloop — a single failure loses all progress.Unlike the new SDK path (which has per-item fallback), if
genai.embed_content()raises on any item here (e.g., rate limit, transient error), the entire batch is lost with no partial results and no REST fallback.At minimum, consider wrapping the loop body in a try/except that falls back to REST for failed items, consistent with the new SDK path.
140-140: Consider a single top-levelimport warningsinstead of repeated inline imports.
warningsis imported conditionally in at least 5 places (lines 140, 162, 196, 246, 321). Since it's a stdlib module with zero cost, a single import at the top of the file would be cleaner and avoid the# noqa: E402dance.Also applies to: 162-162, 196-196
1 participant
Resolves #3693
Related to #908
Proposed change
This PR adds Google Gemini as an alternative LLM provider for NestBot AI Assistant, enabling contributors to run NestBot locally using a free Google Gemini API key as an alternative to OpenAI.
This is part of the larger NestBot AI Assistant feature work tracked in #908.
Changes
GoogleEmbedderclass for Google embeddings usinggemini-embedding-001model (768 dimensions)llm_config.pyto support provider selection (OpenAI/Google) with automatic fallbackembeddings/factory.pyto return Google embedder whenLLM_PROVIDER=googleGOOGLE_API_KEY,GOOGLE_MODEL_NAME,LLM_PROVIDER)pyproject.tomlto includegoogle-genaiextra for CrewAIgenaiandgenerativeai.env.examplewith new Gemini environment variablesTechnical Details
google.genaiSDK (with fallback to deprecatedgoogle.generativeaifor compatibility)gemini-embedding-001(recommended, replaces deprecated models)gemini-2.0-flashConfiguration
Set these environment variables to use Google provider:
For OpenAI provider (default):
Testing
make check-test-backendpasses locallyFiles Changed
backend/apps/ai/common/llm_config.py- Provider selection logicbackend/apps/ai/embeddings/google.py- New Google embedder implementationbackend/apps/ai/embeddings/factory.py- Factory updates for Google providerbackend/settings/base.py- New Django settingsbackend/tests/apps/ai/common/llm_config_test.py- Test coveragebackend/pyproject.toml- Dependency updatesbackend/.env.example- Environment variable documentationcspell/custom-dict.txt- Spell checker updatesChecklist
make check-test-backendlocally: all warnings addressed, tests passed