feat: safe import of optional dependencies #19
Conversation
📝 WalkthroughWalkthroughConverted several unconditional imports to conditional/lazy imports so optional backends/brokers and the CLI are only exposed when their dependencies are present. Added pytest slow marker and Makefile targets for fast/slow test runs. New slow tests validate optional-install behavior in isolated venvs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![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 (5)
pyproject.toml (1)
45-49: LGTM! Consider dependency references to reduce duplication.The
alloptional dependency group correctly aggregates the three optional dependencies. This provides a convenient way for users to install all optional features.Note: If pyproject.toml supports it, you could reference existing groups like
all = ["chicory[redis,cli,rabbitmq]"]to avoid version duplication, but the current approach is explicit and valid.src/chicory/__init__.py (1)
65-83: LGTM! Optional imports properly implemented.The try/except blocks correctly implement optional dependency handling. Components are only exposed in
__all__when their dependencies are installed, preventing import-time failures.Note: The static analysis hint about sorting
__all__(lines 77-80) is a style preference. Since__all__is built dynamically here, the current approach is acceptable. You could consider sorting after all appends if you prefer alphabetical order.tests/unit/test_optional_imports.py (2)
8-8: Remove unused noqa directive.The
# noqa: F401directive appears to be unused according to static analysis. If the import is truly needed for pytest-cov, the noqa may not be necessary since F401 is already ignored in some contexts. Consider removing it or verifying it's needed.🔎 Proposed fix
-import chicory # Import to avoid errors with pytest-cov # noqa: F401 +import chicory # Import to avoid errors with pytest-cov
82-158: LGTM! Well-structured parameterized test with minor cleanup needed.The test comprehensively validates that optional extras install correctly and expose the expected components. The parameterization cleanly covers all scenarios (redis, rabbitmq, all).
Minor cleanup: Remove the unused
# noqa: E501directives on lines 93-94, 103, 109, 113-115 as indicated by static analysis.🔎 Proposed cleanup
[ - "from chicory import broker; print('RedisBroker' in broker.__all__)", # noqa: E501 + "from chicory import broker; print('RedisBroker' in broker.__all__)", - "from chicory import backend; print('RedisBackend' in backend.__all__)", # noqa: E501 + "from chicory import backend; print('RedisBackend' in backend.__all__)", ], ), ( "rabbitmq", [ "from chicory.broker import RabbitMQBroker; print('success')", ], [ - "from chicory import broker; print('RabbitMQBroker' in broker.__all__)", # noqa: E501 + "from chicory import broker; print('RabbitMQBroker' in broker.__all__)", ], ), ( "all", [ - "from chicory.broker import RedisBroker, RabbitMQBroker; print('success')", # noqa: E501 + "from chicory.broker import RedisBroker, RabbitMQBroker; print('success')", "from chicory.backend import RedisBackend; print('success')", ], [ - "from chicory import broker; print('RedisBroker' in broker.__all__)", # noqa: E501 + "from chicory import broker; print('RedisBroker' in broker.__all__)", - "from chicory import broker; print('RabbitMQBroker' in broker.__all__)", # noqa: E501 + "from chicory import broker; print('RabbitMQBroker' in broker.__all__)", - "from chicory import backend; print('RedisBackend' in backend.__all__)", # noqa: E501 + "from chicory import backend; print('RedisBackend' in backend.__all__)", ], ),src/chicory/cli/__init__.py (1)
10-14: Consider extracting the error message to a custom exception class.The static analysis tool suggests avoiding long messages directly in exception instantiation. While this is a stylistic concern, using a custom exception class can improve maintainability and make the error more semantically meaningful.
🔎 Proposed refactor
Define a custom exception at the module level or in a shared exceptions module:
class CliNotInstalledError(RuntimeError): """Raised when CLI dependencies are not installed.""" def __init__(self): super().__init__( "Chicory CLI is not installed. " "Please install it using 'pip install chicory[cli]' " "or with 'pip install chicory[all]'." )Then use it:
except ImportError: - raise RuntimeError( - "Chicory CLI is not installed. " - "Please install it to it using 'pip install chicory[cli]' " - "or with 'pip install chicory[all]'." - ) from None + raise CliNotInstalledError() from NoneBased on static analysis hints from Ruff (TRY003).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Makefilepyproject.tomlsrc/chicory/__init__.pysrc/chicory/app.pysrc/chicory/backend/__init__.pysrc/chicory/broker/__init__.pysrc/chicory/cli/__init__.pytests/unit/test_optional_imports.pytests/unit/test_worker.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:11:37.782Z
Learnt from: dadodimauro
Repo: dadodimauro/chicory PR: 18
File: src/chicory/broker/rabbitmq.py:86-93
Timestamp: 2025-12-29T15:11:37.782Z
Learning: When using aio-pika with a connection pool, acquire a connection from the pool and create a channel from that connection; do not close the connection when releasing it back to the pool. Pool.acquire() returns the connection to the pool for reuse, and RobustConnection objects stay open across acquire/release cycles. This pattern is demonstrated in official aio-pika examples and should be applied to files under src/chicory/broker (e.g., rabbitmq.py).
Applied to files:
src/chicory/broker/__init__.py
🧬 Code graph analysis (2)
src/chicory/__init__.py (4)
src/chicory/app.py (2)
broker(107-108)backend(111-112)src/chicory/broker/rabbitmq.py (1)
RabbitMQBroker(41-783)src/chicory/backend/redis.py (1)
RedisBackend(13-179)src/chicory/broker/redis.py (1)
RedisBroker(19-535)
src/chicory/app.py (3)
src/chicory/backend/base.py (1)
Backend(10-68)src/chicory/broker/base.py (1)
Broker(38-141)src/chicory/types.py (1)
BrokerType(234-236)
🪛 Ruff (0.14.10)
tests/unit/test_optional_imports.py
8-8: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
22-22: subprocess call: check for execution of untrusted input
(S603)
33-33: subprocess call: check for execution of untrusted input
(S603)
60-60: subprocess call: check for execution of untrusted input
(S603)
93-93: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
94-94: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
103-103: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
109-109: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
113-113: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
114-114: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
115-115: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
136-136: subprocess call: check for execution of untrusted input
(S603)
180-180: subprocess call: check for execution of untrusted input
(S603)
src/chicory/cli/__init__.py
10-14: Avoid specifying long messages outside the exception class
(TRY003)
src/chicory/__init__.py
77-80: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run (windows-latest, 3.11)
- GitHub Check: run (windows-latest, 3.12)
🔇 Additional comments (12)
pyproject.toml (1)
79-81: LGTM! Pytest marker properly configured.The
slowmarker is correctly defined and integrates well with the new Makefile targets (test-fastandtest-slow). This enables efficient test segmentation for development workflows.Makefile (1)
21-25: LGTM! Test targets properly configured.The new
test-fastandtest-slowtargets provide an excellent developer experience by enabling quick iteration (skipping slow tests) while still allowing full test runs when needed.tests/unit/test_worker.py (1)
729-729: LGTM! Appropriately marked as slow test.The
@pytest.mark.slowdecorator is correctly applied to this test, which contains a 6-second sleep to simulate a long-running consume loop. This allows developers to skip this test during rapid iteration.src/chicory/app.py (4)
6-7: LGTM! Proper base class imports.The change to import only the base
BackendandBrokerclasses at module level is correct. This ensures the module can load without optional dependencies while still maintaining proper type annotations.
77-78: LGTM! Proper lazy import pattern.The lazy import of
RedisBrokerwithin the_create_brokermethod ensures the Redis dependency is only required when explicitly using a Redis broker. This prevents import-time failures when Redis is not installed.
84-85: LGTM! Proper lazy import pattern.The lazy import of
RabbitMQBrokerwithin the_create_brokermethod ensures the aio-pika dependency is only required when explicitly using a RabbitMQ broker.
98-99: LGTM! Proper lazy import pattern.The lazy import of
RedisBackendwithin the_create_backendmethod maintains consistency with the broker lazy import pattern and ensures Redis is only required when explicitly using a Redis backend.src/chicory/backend/__init__.py (1)
5-12: LGTM! Clean optional import implementation.The conditional export of
RedisBackendis well-implemented. The baseBackendclass remains always available, whileRedisBackendis only exposed when the Redis dependency is installed.src/chicory/broker/__init__.py (1)
5-19: LGTM! Well-structured optional broker imports.The separate try/except blocks for
RabbitMQBrokerandRedisBrokerare correctly implemented. This allows each broker to be independently optional based on installed dependencies, maintaining flexibility for users who may only need one broker type.tests/unit/test_optional_imports.py (2)
39-80: LGTM! Comprehensive base installation test.This test effectively validates that chicory can be imported without optional dependencies and that optional components are correctly absent from
__all__. The use of isolated virtual environments ensures reliable testing of the installation behavior.
160-202: LGTM! CLI installation test properly validates optional CLI dependency.This test effectively validates that the CLI is only available when the appropriate extras are installed. The test correctly handles both the absence case (expecting failure with a specific error) and presence case (expecting help output).
src/chicory/cli/__init__.py (1)
5-14: Verify that immediate import failure is the intended behavior.The current pattern raises
RuntimeErrorimmediately when the CLI dependencies are missing, which prevents the module import from succeeding at all. This is more aggressive than typical optional dependency patterns, which allow imports to succeed but make functionality unavailable.While this provides clear, immediate feedback to users, it prevents graceful degradation. For example, code that conditionally uses CLI features cannot check for availability without triggering an exception.
Consider whether this immediate-failure approach aligns with your design goals, or if a softer pattern would be more appropriate:
__all__ = [] try: from .cli import app __all__.extend(["app"]) except ImportError: # Module import succeeds, but 'app' is unavailable # Users can check availability or handle AttributeError when accessing 'app' pass
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/chicory/cli/__init__.py (1)
12-12: Typo already resolved.The previous review comment about the redundant "to it" in the error message has been successfully addressed. The message now correctly reads "Please install it using...".
🧹 Nitpick comments (2)
src/chicory/cli/__init__.py (1)
10-14: Consider the TRY003 linting suggestion.The static analyzer suggests avoiding long error messages outside the exception class (TRY003). While this is functional, consider extracting the message to a constant or a custom exception class if this pattern is used elsewhere in the codebase.
🔎 Optional refactor to address TRY003
+CLI_NOT_INSTALLED_MSG = ( + "Chicory CLI is not installed. " + "Please install it using 'pip install chicory[cli]' " + "or with 'pip install chicory[all]'." +) + try: from .cli import app __all__.extend(["app"]) except ImportError: - raise RuntimeError( - "Chicory CLI is not installed. " - "Please install it using 'pip install chicory[cli]' " - "or with 'pip install chicory[all]'." - ) from None + raise RuntimeError(CLI_NOT_INSTALLED_MSG) from Nonetests/unit/test_optional_imports.py (1)
102-180: Well-structured test for optional extras.The test effectively validates that optional dependencies (redis, rabbitmq, all) install correctly and their components are properly exposed. The dual verification approach—checking both direct imports and
__all__membership—ensures comprehensive coverage.Note: The unused
noqa: E501directives flagged by static analysis can be safely removed if line-length checking (E501) is not enabled in your linter configuration, but this is a minor style consideration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/chicory/cli/__init__.pytests/unit/test_optional_imports.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/chicory/cli/__init__.py
10-14: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/test_optional_imports.py
9-9: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
41-41: subprocess call: check for execution of untrusted input
(S603)
52-52: subprocess call: check for execution of untrusted input
(S603)
80-80: subprocess call: check for execution of untrusted input
(S603)
113-113: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
114-114: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
123-123: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
129-129: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
133-133: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
134-134: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
135-135: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
157-157: subprocess call: check for execution of untrusted input
(S603)
202-202: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run (windows-latest, 3.14)
- GitHub Check: run (windows-latest, 3.13)
- GitHub Check: run (windows-latest, 3.12)
- GitHub Check: run (windows-latest, 3.11)
🔇 Additional comments (6)
tests/unit/test_optional_imports.py (5)
1-11: LGTM! Clean imports and setup.The import structure is appropriate for isolated venv testing. The explicit chicory import with the noqa comment is correctly documented for pytest-cov compatibility.
15-18: LGTM! Fixture correctly locates project root.The fixture properly navigates from
tests/unit/test_optional_imports.pyup three levels to reach the project root.
20-56: LGTM! Well-structured helper methods with cross-platform support.The helper methods correctly handle platform differences between Windows and Unix systems for venv paths and command execution. The subprocess security warnings (S603) from static analysis are false positives in this test context where all inputs are controlled by the test code.
58-101: LGTM! Comprehensive test for base installation.The test correctly validates that chicory installs and imports without optional dependencies, and verifies that optional components are not exposed in
__all__. The parametrization efficiently covers Redis broker, Redis backend, and RabbitMQ broker scenarios.
181-224: LGTM! Thorough CLI installation testing.The test correctly validates CLI behavior across different installation scenarios:
- Without extras: CLI is unavailable and shows helpful error message
- With
cliorallextras: CLI is available and displays help textThe conditional logic and assertions appropriately handle each scenario.
src/chicory/cli/__init__.py (1)
10-14: Reconsider error handling pattern for missing CLI dependencies.The CLI module raises
RuntimeErrorwhen dependencies are missing, but broker and backend modules use conditional imports with silent degradation (try-except-pass, conditionally updating__all__). This is the only place in the codebase where optional dependencies trigger an import-time error rather than graceful degradation.If raising an error is intentional to enforce CLI dependency installation, that should be documented. Otherwise, align with the conditional export pattern used in broker and backend modules.
| def test_optional_extras_install( | ||
| self, | ||
| project_root: Path, | ||
| extra: str, | ||
| import_commands: list[str], | ||
| import_command_all_names: list[str], | ||
| ) -> None: | ||
| """Test that redis extra installs and works correctly.""" |
There was a problem hiding this comment.
Update docstring to reflect parametrization.
The docstring states "Test that redis extra installs and works correctly," but the test is parametrized over multiple extras (redis, rabbitmq, all). Update the docstring to be more generic.
🔎 Proposed fix
- def test_optional_extras_install(
- self,
- project_root: Path,
- extra: str,
- import_commands: list[str],
- import_command_all_names: list[str],
- ) -> None:
- """Test that redis extra installs and works correctly."""
+ def test_optional_extras_install(
+ self,
+ project_root: Path,
+ extra: str,
+ import_commands: list[str],
+ import_command_all_names: list[str],
+ ) -> None:
+ """Test that optional extras install and work correctly."""📝 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.
| def test_optional_extras_install( | |
| self, | |
| project_root: Path, | |
| extra: str, | |
| import_commands: list[str], | |
| import_command_all_names: list[str], | |
| ) -> None: | |
| """Test that redis extra installs and works correctly.""" | |
| def test_optional_extras_install( | |
| self, | |
| project_root: Path, | |
| extra: str, | |
| import_commands: list[str], | |
| import_command_all_names: list[str], | |
| ) -> None: | |
| """Test that optional extras install and work correctly.""" |
🤖 Prompt for AI Agents
In tests/unit/test_optional_imports.py around lines 141 to 148, the test
docstring specifically mentions "redis" but the test is parametrized over
multiple extras (redis, rabbitmq, all); update the docstring to a generic
description reflecting parametrization such as "Test that optional extras
install and work correctly" or "Test that each optional extra installs and works
correctly" so it accurately describes all parameter cases.
Summary by CodeRabbit
New Features
test-fastandtest-slowcommands to run targeted test subsets.Tests
✏️ Tip: You can customize this high-level summary in your review settings.