fix: make MCP package import optional to prevent errors when not installed#1096
Conversation
…alled Wrap mcp imports in try/except blocks in mcp.py, mcp_sse.py, and mcp_http_stream.py. Add MCP_AVAILABLE flag to check availability at runtime. When MCP class is instantiated without the mcp package installed, raise a clear ImportError with installation instructions. Closes MervinPraison#1091 Signed-off-by: majiayu000 <1835304752@qq.com>
📝 WalkthroughWalkthroughThis change makes MCP (Model Context Protocol) dependencies optional by introducing defensive import wrappers across three MCP-related modules, adding availability flags, and inserting pre-flight checks in initialization methods to raise helpful ImportError messages when MCP is unavailable. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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. Comment |
Summary of ChangesHello @majiayu000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly makes the mcp package an optional dependency by wrapping imports in try-except blocks and raising an ImportError at runtime if the package is not installed. This is a good approach that improves the user experience for those not using MCP features. The changes are applied consistently across all relevant modules. However, the new test suite is incomplete. While it verifies that the application doesn't crash on import, it fails to test the new error-raising mechanism when the optional dependencies are missing. I've added suggestions to include these crucial tests to ensure the feature is robust and works as described in the test plan.
| def test_mcp_lazy_loading(self): | ||
| """Test that MCP is lazily loaded.""" | ||
| from praisonaiagents import MCP | ||
| # MCP should either be available or None (not raise ImportError on access) | ||
| # The actual ImportError should only be raised when trying to instantiate | ||
| # if mcp package is not installed | ||
|
|
There was a problem hiding this comment.
The test_mcp_lazy_loading test is currently a placeholder without any assertions. It's crucial to verify that instantiating the MCP-related classes correctly raises an ImportError when their dependencies are missing. This suggestion replaces the placeholder with comprehensive tests for this behavior, covering MCP, SSEMCPClient, and HTTPStreamMCPClient.
def test_mcp_instantiation_raises_error_if_mcp_unavailable(self):
"""Test that instantiating MCP classes raises ImportError if mcp is not installed."""
from unittest.mock import patch
with patch.dict('sys.modules', {'mcp': None, 'mcp.client.stdio': None, 'mcp.client.sse': None}):
import importlib
# Test MCP class
from praisonaiagents.mcp import mcp
importlib.reload(mcp)
assert not mcp.MCP_AVAILABLE
with pytest.raises(ImportError, match="MCP .* not installed"):
mcp.MCP("some command")
importlib.reload(mcp) # Restore
# Test SSEMCPClient class
from praisonaiagents.mcp import mcp_sse
importlib.reload(mcp_sse)
assert not mcp_sse.MCP_AVAILABLE
with pytest.raises(ImportError, match="MCP .* not installed"):
mcp_sse.SSEMCPClient("http://localhost/sse")
importlib.reload(mcp_sse) # Restore
# Test HTTPStreamMCPClient class for MCP
from praisonaiagents.mcp import mcp_http_stream
importlib.reload(mcp_http_stream)
assert not mcp_http_stream.MCP_AVAILABLE
with pytest.raises(ImportError, match="MCP .* not installed"):
mcp_http_stream.HTTPStreamMCPClient("http://localhost/stream")
importlib.reload(mcp_http_stream) # Restore
def test_http_stream_instantiation_raises_error_if_aiohttp_unavailable(self):
"""Test that instantiating HTTPStreamMCPClient raises ImportError if aiohttp is not installed."""
from unittest.mock import patch
with patch.dict('sys.modules', {'aiohttp': None}):
import importlib
from praisonaiagents.mcp import mcp_http_stream
importlib.reload(mcp_http_stream)
with pytest.raises(ImportError, match="aiohttp is required"):
mcp_http_stream.HTTPStreamMCPClient("http://localhost/stream")
importlib.reload(mcp_http_stream) # Restore| import sys | ||
| import pytest |
There was a problem hiding this comment.
To properly test for missing optional dependencies by mocking imports, we need to use patch from unittest.mock. Please add this import.
| import sys | |
| import pytest | |
| import sys | |
| import pytest | |
| from unittest.mock import patch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/praisonai-agents/praisonaiagents/mcp/mcp_http_stream.py (1)
516-528: Clear error handling for missing dependencies.The preflight checks correctly raise ImportError with helpful installation instructions when MCP or aiohttp packages are unavailable. This ensures users get clear guidance rather than cryptic errors.
Note: Static analysis suggests using custom exception classes for long messages (TRY003), but the current inline approach is clear and appropriate for this use case.
src/praisonai-agents/tests/test_mcp_optional_import.py (2)
23-28: Consider enhancing the lazy loading test.The test currently only imports the MCP class but doesn't verify the lazy loading behavior (i.e., that ImportError is raised on instantiation, not on import, when MCP is unavailable). Consider adding a test that attempts to instantiate MCP and verifies the error behavior.
Example enhancement
def test_mcp_lazy_loading(self): """Test that MCP is lazily loaded.""" from praisonaiagents import MCP - # MCP should either be available or None (not raise ImportError on access) - # The actual ImportError should only be raised when trying to instantiate - # if mcp package is not installed + + # Import should succeed even if MCP package is not installed + assert MCP is not None + + # Try to instantiate - this should raise ImportError if MCP not available + try: + from praisonaiagents.mcp.mcp import MCP_AVAILABLE + if not MCP_AVAILABLE: + with pytest.raises(ImportError, match="MCP.*not installed"): + MCP("test command") + except ImportError: + pass # MCP module itself couldn't be imported
30-55: Module import tests are functional.These tests effectively verify that MCP-related modules can be imported and expose the MCP_AVAILABLE flag. The tests correctly handle both scenarios (MCP available or not).
As an optional improvement, consider adding assertions about the exception type or message when ImportError is caught, to distinguish between missing MCP dependencies and potential syntax errors in the modules themselves.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/praisonai-agents/praisonaiagents/mcp/mcp.pysrc/praisonai-agents/praisonaiagents/mcp/mcp_http_stream.pysrc/praisonai-agents/praisonaiagents/mcp/mcp_sse.pysrc/praisonai-agents/tests/test_mcp_optional_import.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/praisonai-agents/tests/test_mcp_optional_import.py (1)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
MCP(150-653)
🪛 Ruff (0.14.10)
src/praisonai-agents/praisonaiagents/mcp/mcp.py
208-211: Avoid specifying long messages outside the exception class
(TRY003)
src/praisonai-agents/praisonaiagents/mcp/mcp_http_stream.py
518-521: Avoid specifying long messages outside the exception class
(TRY003)
525-528: Avoid specifying long messages outside the exception class
(TRY003)
src/praisonai-agents/praisonaiagents/mcp/mcp_sse.py
180-183: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
src/praisonai-agents/praisonaiagents/mcp/mcp_http_stream.py (1)
19-29: LGTM! Optional imports correctly implemented.The try/except blocks properly guard MCP and aiohttp imports, setting appropriate flags and None values when packages are unavailable. This enables the module to be imported without these dependencies installed.
src/praisonai-agents/praisonaiagents/mcp/mcp.py (2)
14-22: LGTM! Optional MCP imports correctly implemented.The try/except block properly guards all MCP imports, enabling the module to be imported without the MCP package installed while setting appropriate availability flags.
206-211: Clear error handling when MCP is unavailable.The preflight check correctly raises ImportError with installation instructions when attempting to use MCP functionality without the package installed, aligning with the PR objective.
src/praisonai-agents/praisonaiagents/mcp/mcp_sse.py (2)
14-21: LGTM! Optional MCP imports correctly implemented.The try/except block properly guards MCP SSE client imports, maintaining consistency with the optional import pattern used across all MCP modules.
178-183: Clear error handling maintains consistency.The preflight check in SSEMCPClient follows the same pattern as other MCP modules, providing clear error messages when the MCP package is unavailable.
src/praisonai-agents/tests/test_mcp_optional_import.py (2)
12-21: LGTM! Basic import tests are well-structured.These tests effectively verify that the core praisonaiagents package and Agent class can be imported without MCP installed, directly addressing the issue from #1091.
57-67: LGTM! Test effectively validates the fix for issue #1091.This test directly addresses the original issue by verifying that basic agent code works without MCP installed, which was the main objective of the PR.
2 participants
User description
Summary
Test plan
Closes #1091
PR Type
Bug fix
Description
Wrap MCP imports in try/except blocks across three modules
Add MCP_AVAILABLE flag for runtime availability checking
Raise clear ImportError with installation instructions when MCP instantiated without package
Add comprehensive test suite for optional MCP import functionality
Diagram Walkthrough
File Walkthrough
mcp.py
Add optional MCP import with runtime validationsrc/praisonai-agents/praisonaiagents/mcp/mcp.py
mcpandmcp.client.stdioimports in try/except blockMCP_AVAILABLEflag to track import successNoneon import failure__init__to raise clear ImportError withinstallation instructions
mcp_sse.py
Add optional MCP SSE import with runtime validationsrc/praisonai-agents/praisonaiagents/mcp/mcp_sse.py
mcp.ClientSessionandmcp.client.sseimports in try/exceptblock
MCP_AVAILABLEflag to track import successNoneon import failure__init__to raise clear ImportError withinstallation instructions
mcp_http_stream.py
Add optional MCP HTTP stream import with dual validationsrc/praisonai-agents/praisonaiagents/mcp/mcp_http_stream.py
mcp.ClientSessionimport in try/except blockMCP_AVAILABLEflag to track import successaiohttpimport failure from raising to setting toNone__init__for both MCP and aiohttp availabilitydependencies missing
test_mcp_optional_import.py
Add test suite for optional MCP import functionalitysrc/praisonai-agents/tests/test_mcp_optional_import.py
imports
MCP_AVAILABLEflag presence in modulesSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.