Conversation
WalkthroughThe changes update the optional dependencies for the "gemini" group in the configuration file by expanding the list to include "google-api-core~=2.25,<3.0" alongside existing packages. In the source code, a function in ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🔭 Outside diff range comments (2)
src/draive/gemini/lmm.py (1)
352-367: Add docstring & make the match case-insensitiveThe mapping is correct, but:
- A short docstring would silence the lint warning and clarify intent.
- Consider normalising the input (
resolution.casefold()) so callers can pass"Medium"/"MEDIUM"etc. without surprisingNone.-def resolution_as_media_resolution( - resolution: Literal["low", "medium", "high"] | Missing, +def resolution_as_media_resolution( + resolution: Literal["low", "medium", "high"] | Missing,src/draive/gemini/lmm_generation.py (1)
195-254: Rate-limit handling always retries immediatelyRaising
RateLimitError(retry_after=0)causes upstream middleware to retry with no delay, which can hammer the API.
If theResourceExhaustedmessage includes a suggested wait time, parse it, otherwise fall back to an exponential back-off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
pyproject.toml(1 hunks)src/draive/gemini/lmm.py(2 hunks)src/draive/gemini/lmm_generation.py(9 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/draive/gemini/lmm.py
[convention] 352-352: Missing function or method docstring
(C0116)
src/draive/gemini/lmm_generation.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'google.api_core.exceptions'
(E0401)
[error] 5-5: No name 'api_core' in module 'google'
(E0611)
[error] 46-46: Cannot import 'draive.multimodal.content' due to 'expected '(' (draive.multimodal.content, line 100)'
(E0001)
[error] 46-46: No name 'content' in module 'draive.multimodal'
(E0611)
[convention] 82-82: Missing function or method docstring
(C0116)
[refactor] 82-82: Too many arguments (7/5)
(R0913)
[refactor] 82-82: Too many local variables (16/15)
(R0914)
[refactor] 133-193: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 195-195: Too many arguments (18/5)
(R0913)
[refactor] 195-195: Too many positional arguments (18/5)
(R0917)
[refactor] 195-195: Too many local variables (32/15)
(R0914)
[refactor] 195-195: Too many branches (13/12)
(R0912)
[refactor] 333-360: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 362-362: Too many arguments (18/5)
(R0913)
[refactor] 362-362: Too many positional arguments (18/5)
(R0917)
[refactor] 362-362: Too many local variables (21/15)
(R0914)
[refactor] 414-414: Too many branches (16/12)
(R0912)
🪛 GitHub Actions: CI
src/draive/gemini/lmm_generation.py
[error] 5-5: Pyright: Import "google.api_core.exceptions" could not be resolved (reportMissingImports)
🔇 Additional comments (1)
pyproject.toml (1)
38-42: Verifygoogle-api-core~=2.25actually exists and is compatible
~=pins you to the 2.25. series; if the latest released major-minor is lower (e.g. 2.20) or higher (e.g. 2.30), the resolver will fail.
Double-check PyPI and, if needed, relax the pin to"google-api-core>=2.25,<3"or to the latest available minor.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/draive/gemini/lmm_generation.py (2)
139-170: Request-builder logic still duplicated – extract a helper to DRY the code.
The two hugeconfig={…}dictionaries in_completionand_completion_streamremain almost identical, keeping cyclomatic complexity high and risking drift.
Refactor into a private_build_request_config(**params)used by both paths.
420-547: Ensure the streaming iterator is closed to avoid leaked transports.
The innerasync forexits viabreakor exception without callingcompletion_stream.aclose().
Wrap the loop intry / finallyand close:- async for completion_chunk in completion_stream: + try: + async for completion_chunk in completion_stream: + ... + finally: + await completion_stream.aclose()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/draive/gemini/lmm_generation.py(9 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/draive/gemini/lmm_generation.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 52-52: Cannot import 'draive.multimodal.content' due to 'expected '(' (draive.multimodal.content, line 100)'
(E0001)
[error] 52-52: No name 'content' in module 'draive.multimodal'
(E0611)
[convention] 88-88: Missing function or method docstring
(C0116)
[refactor] 88-88: Too many arguments (7/5)
(R0913)
[refactor] 88-88: Too many local variables (16/15)
(R0914)
[refactor] 139-199: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 201-201: Too many arguments (18/5)
(R0913)
[refactor] 201-201: Too many positional arguments (18/5)
(R0917)
[refactor] 201-201: Too many local variables (32/15)
(R0914)
[refactor] 201-201: Too many branches (13/12)
(R0912)
[warning] 261-261: Catching previously caught exception type Exception
(W0705)
[refactor] 339-366: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 368-368: Too many arguments (18/5)
(R0913)
[refactor] 368-368: Too many positional arguments (18/5)
(R0917)
[refactor] 368-368: Too many local variables (21/15)
(R0914)
[refactor] 420-420: Too many branches (16/12)
(R0912)
🪛 GitHub Actions: CI
src/draive/gemini/lmm_generation.py
[error] 6-6: Import "google.api_core.exceptions" could not be resolved (reportMissingImports)
🔇 Additional comments (1)
src/draive/gemini/lmm_generation.py (1)
5-10: Import guard looks good – thanks for fixing the optional-dependency issue.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/draive/gemini/lmm_generation.py (2)
5-13: Optional-dependency guard implemented correctly – nice!
TheTYPE_CHECKINGgate and runtime fallback forResourceExhaustedresolve the missing-import issue raised earlier.
175-204: Drop redundantelseafter earlyreturn
Since theif stream:branch already returns, theelse:block is superfluous. Removing it and de-indenting the synchronous path simplifies control flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/draive/gemini/lmm_generation.py(8 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/draive/gemini/lmm_generation.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 57-57: Cannot import 'draive.multimodal.content' due to 'expected '(' (draive.multimodal.content, line 100)'
(E0001)
[convention] 57-57: Import "from draive.multimodal.content import MultimodalContent, MultimodalContentElement" should be placed at the top of the module
(C0413)
[error] 57-57: No name 'content' in module 'draive.multimodal'
(E0611)
[convention] 58-58: Import "from draive.utils import RateLimitError" should be placed at the top of the module
(C0413)
[convention] 93-93: Missing function or method docstring
(C0116)
[refactor] 93-93: Too many arguments (7/5)
(R0913)
[refactor] 93-93: Too many local variables (16/15)
(R0914)
[refactor] 144-204: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (18/5)
(R0913)
[refactor] 206-206: Too many positional arguments (18/5)
(R0917)
[refactor] 206-206: Too many local variables (32/15)
(R0914)
[warning] 256-256: Catching previously caught exception type Exception
(W0705)
[refactor] 334-361: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 206-206: Too many branches (13/12)
(R0912)
[refactor] 363-363: Too many arguments (18/5)
(R0913)
[refactor] 363-363: Too many positional arguments (18/5)
(R0917)
[refactor] 363-363: Too many local variables (21/15)
(R0914)
[warning] 408-408: Catching previously caught exception type Exception
(W0705)
[warning] 538-540: The except handler raises immediately
(W0706)
[refactor] 411-411: Too many branches (18/12)
(R0912)
[refactor] 549-549: Too many arguments (14/5)
(R0913)
wporoslo
force-pushed
the
feat-gemini-streaming
branch
from
6ffeea9 to
9172517
Compare
wporoslo
force-pushed
the
feat-gemini-streaming
branch
from
9172517 to
9a02e4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/draive/gemini/lmm.py (1)
352-365: Add a short docstring
resolution_as_media_resolution()is public (exported in__all__) but still lacks even a one-liner.
Document accepted literals and the fact thatMissing|NonereturnsNoneso downstream can rely on it.
♻️ Duplicate comments (3)
src/draive/gemini/lmm_generation.py (3)
181-196: Drop the redundantelseThe
if stream:branch returns immediately, so theelse:is unnecessary noise:- if stream: - return … - else: - return … + if stream: + return … + return …
245-256: Ensure the gRPC/HTTP stream is closed
completion_streamis never explicitly closed; breaking out of the loop leaks the underlying transport.- try: - completion_stream = … - … - async def stream(): - async for chunk in self._stream_processor(…): - yield chunk + try: + completion_stream = … + … + async def stream(): + try: + async for chunk in self._stream_processor(…): + yield chunk + finally: + await completion_stream.aclose()
5-10: Use a semantic fallback forResourceExhaustedShadowing
ResourceExhaustedwith plainExceptionloses its semantic meaning (rate-limit).
Safer fallback:-ResourceExhausted = Exception +class ResourceExhausted(Exception): # pragma: no cover + """Stub raised when google-api-core is absent."""so downstream
except ResourceExhaustedkeeps working.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
pyproject.toml(1 hunks)src/draive/gemini/lmm.py(2 hunks)src/draive/gemini/lmm_generation.py(8 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/draive/gemini/lmm.py
[convention] 352-352: Missing function or method docstring
(C0116)
src/draive/gemini/lmm_generation.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 53-53: Cannot import 'draive.multimodal.content' due to 'expected '(' (draive.multimodal.content, line 100)'
(E0001)
[error] 53-53: No name 'content' in module 'draive.multimodal'
(E0611)
[refactor] 89-89: Too many arguments (7/5)
(R0913)
[refactor] 89-89: Too many local variables (17/15)
(R0914)
[refactor] 181-196: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 198-198: Too many arguments (6/5)
(R0913)
[refactor] 198-198: Too many positional arguments (6/5)
(R0917)
[warning] 216-216: Catching previously caught exception type Exception
(W0705)
[refactor] 236-236: Too many arguments (6/5)
(R0913)
[refactor] 236-236: Too many positional arguments (6/5)
(R0917)
[warning] 254-254: Catching previously caught exception type Exception
(W0705)
[refactor] 297-324: Unnecessary "elif" after "break", remove the leading "el" from "elif"
(R1723)
[refactor] 265-265: Too many branches (14/12)
(R0912)
[refactor] 367-370: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 407-428: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[warning] 404-404: Unused argument 'is_streaming'
(W0613)
[refactor] 468-468: Too many arguments (14/5)
(R0913)
🔇 Additional comments (1)
src/draive/gemini/lmm_generation.py (1)
169-173: Verify modality names sent to the API
[m.name for m in response_modalities]yieldsTEXT,IMAGE, … whereas Gemini expects lowercase strings ("text","image").
If that’s the case, usem.value.lower()or a dedicated mapping to avoid 400 replies.
wporoslo
force-pushed
the
feat-gemini-streaming
branch
from
946e8b3 to
938d67c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/draive/gemini/lmm_generation.py (1)
256-262: Ensure the streaming connection is properly closed.The
stream()generator doesn't ensure thatcompletion_streamis closed when iteration completes or is interrupted. This can lead to resource leaks.Wrap the stream processing in a try-finally block to ensure cleanup:
-async def stream(): - async for chunk in self._stream_processor( - completion_stream, model, functions, output_decoder - ): - yield chunk +async def stream(): + try: + async for chunk in self._stream_processor( + completion_stream, model, functions, output_decoder + ): + yield chunk + finally: + # Ensure the stream is closed even if iteration is interrupted + if hasattr(completion_stream, 'aclose'): + await completion_stream.aclose()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/draive/gemini/lmm_generation.py(8 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/draive/gemini/lmm_generation.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 55-55: Cannot import 'draive.multimodal.content' due to 'expected '(' (draive.multimodal.content, line 100)'
(E0001)
[error] 55-55: No name 'content' in module 'draive.multimodal'
(E0611)
[refactor] 91-91: Too many arguments (7/5)
(R0913)
[refactor] 91-91: Too many local variables (17/15)
(R0914)
[refactor] 199-199: Too many arguments (6/5)
(R0913)
[refactor] 199-199: Too many positional arguments (6/5)
(R0917)
[refactor] 235-235: Too many arguments (6/5)
(R0913)
[refactor] 235-235: Too many positional arguments (6/5)
(R0917)
[refactor] 296-323: Unnecessary "elif" after "break", remove the leading "el" from "elif"
(R1723)
[refactor] 264-264: Too many branches (14/12)
(R0912)
[refactor] 366-369: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 405-426: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 466-466: Too many arguments (14/5)
(R0913)
🔇 Additional comments (4)
src/draive/gemini/lmm_generation.py (4)
5-12: Import guard properly implemented!The ResourceExhausted exception is now correctly handled with a try-except fallback, ensuring the code works even when the optional google-api-core dependency is not installed.
158-197: Excellent refactoring with proper delegation pattern!The method now cleanly delegates to specialized methods for streaming and non-streaming cases, and the common request building logic is properly extracted into
_build_request. This addresses the previous concerns about code duplication and complexity.
428-463: Well-implemented tool call accumulation logic!The method correctly handles merging partial tool calls with the same identifier, which is essential for streaming scenarios. The logic properly handles both dictionary and non-dictionary arguments.
466-509: Clean consolidation of request building logic!The
_build_requestfunction successfully extracts and consolidates all request configuration logic, addressing the previous concerns about code duplication between streaming and non-streaming paths.
2 participants
No description provided.