Conversation
|
Warning Rate limit exceeded@KaQuMiQ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThis change introduces new exception classes and a runtime-checkable protocol for output decoding to the ✨ 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: 6
🔭 Outside diff range comments (1)
src/draive/lmm/helpers.py (1)
74-83: Add error handling for model decodingThe model conversion could fail if the content doesn't match the expected schema. Consider adding error handling.
def _prepare_model_output_conversion( model: type[DataModel], ) -> LMMOutputDecoder: def _model_output_conversion( content: MultimodalContent, ) -> MultimodalContent: - return MultimodalContent.of(model.from_json(content.to_str())) + try: + return MultimodalContent.of(model.from_json(content.to_str())) + except ValueError as exc: + from draive.lmm.types import LMMOutputInvalid + raise LMMOutputInvalid(f"Failed to decode {model.__name__} output") from exc return _model_output_conversion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/draive/__init__.py(2 hunks)src/draive/gemini/config.py(1 hunks)src/draive/gemini/lmm.py(1 hunks)src/draive/gemini/lmm_generation.py(6 hunks)src/draive/gemini/types.py(1 hunks)src/draive/lmm/__init__.py(4 hunks)src/draive/lmm/helpers.py(1 hunks)src/draive/lmm/types.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Follow Ruff import ordering (standard library, third party, local) Us...
**/*.py: Follow Ruff import ordering (standard library, third party, local)
Use Python 3.12+ type features (type unions with |, generic syntax)
Use base and abstract types like Sequence or Iterable instead of concrete types
Use custom exceptions for specific errors
Format code with Ruff
Run linters (Ruff + Bandit + Pyright strict mode)
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/draive/gemini/types.pysrc/draive/gemini/config.pysrc/draive/__init__.pysrc/draive/lmm/helpers.pysrc/draive/gemini/lmm.pysrc/draive/lmm/types.pysrc/draive/lmm/__init__.pysrc/draive/gemini/lmm_generation.py
`**/__init__.py`: Put exported symbols into __init__.py
**/__init__.py: Put exported symbols into init.py
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/draive/__init__.pysrc/draive/lmm/__init__.py
🧠 Learnings (4)
src/draive/gemini/types.py (1)
Learnt from: CR
PR: miquido/draive#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T09:39:42.979Z
Learning: Applies to **/*.py : Use custom exceptions for specific errors
src/draive/__init__.py (1)
Learnt from: KaQuMiQ
PR: miquido/draive#338
File: src/draive/bedrock/lmm_generation.py:102-107
Timestamp: 2025-06-16T09:44:08.688Z
Learning: LMMInstruction is a type alias for str in the draive codebase, so LMMInstruction and str are equivalent types and can be used interchangeably.
src/draive/lmm/types.py (1)
Learnt from: KaQuMiQ
PR: miquido/draive#338
File: src/draive/bedrock/lmm_generation.py:102-107
Timestamp: 2025-06-16T09:44:08.688Z
Learning: LMMInstruction is a type alias for str in the draive codebase, so LMMInstruction and str are equivalent types and can be used interchangeably.
src/draive/lmm/__init__.py (1)
Learnt from: KaQuMiQ
PR: miquido/draive#338
File: src/draive/bedrock/lmm_generation.py:102-107
Timestamp: 2025-06-16T09:44:08.688Z
Learning: LMMInstruction is a type alias for str in the draive codebase, so LMMInstruction and str are equivalent types and can be used interchangeably.
🧬 Code Graph Analysis (5)
src/draive/gemini/types.py (1)
src/draive/lmm/types.py (1)
LMMException(121-122)
src/draive/__init__.py (1)
src/draive/lmm/types.py (2)
LMMOutputInvalid(129-130)LMMOutputLimit(125-126)
src/draive/lmm/helpers.py (3)
src/draive/lmm/types.py (10)
LMMOutputDecoder(184-188)of(86-109)of(145-154)of(165-174)of(196-212)of(223-232)of(240-254)of(264-275)of(289-300)of(355-365)src/draive/multimodal/content.py (2)
MultimodalContent(24-206)media(59-69)src/draive/parameters/model.py (1)
DataModel(386-756)
src/draive/lmm/types.py (1)
src/draive/multimodal/content.py (1)
MultimodalContent(24-206)
src/draive/lmm/__init__.py (2)
src/draive/lmm/helpers.py (1)
lmm_output_decoder(8-35)src/draive/lmm/types.py (3)
LMMOutputDecoder(184-188)LMMOutputInvalid(129-130)LMMOutputLimit(125-126)
🪛 Pylint (3.3.7)
src/draive/gemini/types.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Cannot import 'draive.lmm.types' due to 'invalid syntax (draive.lmm.types, line 73)'
(E0001)
[error] 1-1: No name 'types' in module 'draive.lmm'
(E0611)
[convention] 6-6: Missing class docstring
(C0115)
src/draive/lmm/helpers.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Cannot import 'draive.lmm.types' due to 'invalid syntax (draive.lmm.types, line 73)'
(E0001)
[error] 1-1: No name 'types' in module 'draive.lmm'
(E0611)
[error] 3-3: Cannot import 'draive.parameters.model' due to 'expected '(' (draive.parameters.model, line 46)'
(E0001)
[error] 3-3: No name 'model' in module 'draive.parameters'
(E0611)
[convention] 8-8: Missing function or method docstring
(C0116)
[refactor] 8-8: Too many return statements (8/6)
(R0911)
src/draive/gemini/lmm.py
[convention] 1-1: Missing module docstring
(C0114)
src/draive/lmm/__init__.py
[convention] 1-1: Missing module docstring
(C0114)
src/draive/gemini/lmm_generation.py
[error] 46-46: Cannot import 'draive.lmm.types' due to 'invalid syntax (draive.lmm.types, line 73)'
(E0001)
[error] 46-46: No name 'types' in module 'draive.lmm'
(E0611)
[refactor] 140-140: Too many branches (17/12)
(R0912)
[refactor] 265-292: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[warning] 626-626: TODO: handle specific tool selection?
(W0511)
[refactor] 328-328: Too many branches (15/12)
(R0912)
[refactor] 523-523: Too many local variables (17/15)
(R0914)
[refactor] 523-523: Too many statements (57/50)
(R0915)
🔇 Additional comments (14)
src/draive/gemini/config.py (1)
25-25: LGTM! Clean addition of new configuration field.The
thinking_budgetfield follows the established pattern of optional configuration parameters and uses proper Python 3.12+ type union syntax.src/draive/gemini/types.py (2)
1-1: Import alignment with unified exception hierarchy.The import of
LMMExceptionproperly aligns with the refactoring to use a unified exception hierarchy across the LMM modules.
6-7: LGTM! Exception hierarchy properly unified.Changing
GeminiExceptionto inherit fromLMMExceptioncorrectly integrates Gemini-specific errors into the common LMM error handling structure, which aligns with the PR objective of refining error handling.src/draive/__init__.py (2)
138-139: New exception classes properly imported.The import of
LMMOutputInvalidandLMMOutputLimitcorrectly exposes the new exception types from the LMM module at the package level.
301-302: Exception classes properly exported in all.The new exception classes are correctly added to the
__all__list, making them part of the public API as intended.src/draive/lmm/__init__.py (3)
1-1: Helper function properly imported.The import of
lmm_output_decodercorrectly exposes the new output decoding functionality at the package level.
12-14: New types properly imported.The import of
LMMOutputDecoder,LMMOutputInvalid, andLMMOutputLimitcorrectly exposes the new protocol and exception types at the package level.
51-53: New API elements properly exported.All new types and the helper function are correctly added to the
__all__list, properly extending the public API surface of the lmm package.Also applies to: 78-78
src/draive/lmm/types.py (3)
36-38: New exception types properly added to exports.The new exception classes are correctly added to the
__all__list for proper public API exposure.
125-130: Clean exception class definitions.The new exception classes
LMMOutputLimitandLMMOutputInvalidfollow the established pattern of inheriting fromLMMExceptionand provide clear separation of error types for output handling scenarios.
183-188: Well-defined output decoder protocol.The
LMMOutputDecoderprotocol provides a clean interface for output decoding functionality with proper runtime checking enabled. The protocol signature is clear and follows the multimodal content processing pattern used throughout the codebase.src/draive/gemini/lmm.py (1)
231-252: Document rationale for disabled safety settingsAll safety settings are disabled, which could allow harmful content. Please add a comment explaining why this is necessary, or consider making it configurable.
+# Safety settings are disabled because [explain rationale] +# Consider making this configurable based on use case requirements DISABLED_SAFETY_SETTINGS: list[SafetySettingDict] = [src/draive/gemini/lmm_generation.py (2)
170-293: Excellent error handling and observability improvements!The refactored error handling provides clear error messages, detailed logging, and proper handling of edge cases. The safety rating logging and finish reason handling are particularly well done.
688-693: Good implementation of thinking_budget supportThe thinking_budget configuration is properly integrated with appropriate conditional logic.
KaQuMiQ
force-pushed
the
feature/gemini_handling
branch
4 times, most recently
from
9cdef5b to
c3688e8
Compare
KaQuMiQ
force-pushed
the
feature/gemini_handling
branch
from
c3688e8 to
f73af54
Compare
1 participant
No description provided.