Add MCP integration for stock price retrieval #429
Conversation
- Introduced multiple new files for MCP agent functionality, including `mcp-agents-detailed.py`, `mcp-agents.py`, `mcp-basic.py`, `mcp-multiagents.py`, and `mcp-test.py`. - Implemented a dedicated `MCP` class for managing connections and tool execution. - Created an asynchronous function to retrieve stock prices using MCP tools. - Added example agents for stock price checking and web searching. - Updated `pyproject.toml` and `uv.lock` to reflect version increment to 0.0.66. - Ensured backward compatibility and improved tool function generation for better usability.
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces multiple new agent files for handling stock price inquiries, Airbnb bookings, and web search functionalities. Several asynchronous mechanisms and wrappers have been added to support integration with MCP servers, NPX commands, and the Google GenAI API. New classes and functions enhance tool initialization, execution, and error handling. Additionally, the project version has been updated, and package exports have been expanded to include the MCP class. Changes
Sequence Diagram(s)Stock Price Inquiry FlowsequenceDiagram
participant User
participant Agent
participant MCP
participant StockTool
User->>Agent: "What is the stock price of Tesla?"
Agent->>MCP: Invoke tool for stock price retrieval
MCP->>StockTool: Execute stock price command
StockTool-->>MCP: Return stock price result
MCP-->>Agent: Send back result
Agent->>User: Display Tesla stock price
Airbnb Booking FlowsequenceDiagram
participant User
participant Agent
participant MCP
participant AirbnbService
User->>Agent: "Book an apartment in Paris for two nights"
Agent->>MCP: Invoke NPX command for Airbnb booking
MCP->>AirbnbService: Execute booking command with parameters
AirbnbService-->>MCP: Return booking confirmation
MCP-->>Agent: Relay confirmation details
Agent->>User: Present booking details
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @MervinPraison, 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!
Summary of Changes
This pull request introduces Model Context Protocol (MCP) integration to the praisonai-agents library, enabling agents to utilize external tools for tasks like retrieving stock prices. I've added new files (mcp-agents-detailed.py, mcp-agents.py, mcp-basic.py, mcp-multiagents.py, and mcp-test.py) to demonstrate MCP agent functionality. The core of the integration is the MCP class, which manages connections and tool execution. I've also included examples of agents that use MCP to check stock prices and search the web. Additionally, I've updated pyproject.toml and uv.lock to version 0.0.66 and ensured backward compatibility while improving tool function generation.
Highlights
- MCP Integration: Introduced the
MCPclass for managing connections to MCP servers and executing tools within PraisonAI agents. - Example Agents: Added example agents for stock price checking and web searching using MCP tools.
- Tool Function Generation: Improved tool function generation to enhance usability and ensure backward compatibility.
- Version Update: Incremented the package version to 0.0.66 in
pyproject.tomlanduv.lock.
Changelog
Click here to see the changelog
- src/praisonai-agents/mcp-agents-detailed.py
- Created a new file to demonstrate detailed MCP agent usage for checking stock prices, utilizing the
AgentandMCPclasses frompraisonaiagents. - The agent is configured with instructions to act as a helpful assistant that can check stock prices and perform other tasks using available tools.
- The agent is initialized with the 'gpt-4o-mini' LLM and uses an MCP tool pointing to a stock price application.
- The agent is started with the question 'What is the stock price of Tesla?'.
- Created a new file to demonstrate detailed MCP agent usage for checking stock prices, utilizing the
- src/praisonai-agents/mcp-agents.py
- Created a new file to demonstrate basic MCP agent usage for checking stock prices, utilizing the
AgentandMCPclasses frompraisonaiagents. - The agent is configured with instructions to act as a helpful assistant that can check stock prices and perform other tasks using available tools.
- The agent is initialized with the 'gpt-4o-mini' LLM and uses an MCP tool pointing to a stock price application.
- The agent is started with the question 'What is the stock price of Tesla?'.
- Created a new file to demonstrate basic MCP agent usage for checking stock prices, utilizing the
- src/praisonai-agents/mcp-basic.py
- Created a new file to demonstrate basic MCP integration for retrieving stock prices using asynchronous functions and custom tools.
- Defines a server configuration pointing to a stock price application using
StdioServerParameters. - Implements an asynchronous function
get_stock_priceto retrieve stock prices using MCP tools, including initializing a client session, listing available tools, and calling a tool to get the stock price for a given symbol. - Creates a custom tool
stock_price_toolthat wraps the asynchronous function to be used by the agent. - Creates an agent with instructions to check stock prices using the
stock_price_tooland starts the agent with the question 'What is the stock price of Tesla?'.
- src/praisonai-agents/mcp-multiagents.py
- Created a new file to demonstrate the usage of multiple agents with MCP integration for different tasks, including stock price checking and web searching.
- Defines a
stock_agentthat checks stock prices using an MCP tool pointing to a stock price application. - Defines a
search_agentthat searches the web using an MCP tool configured with Brave Search. - Starts both agents with questions related to their respective tasks: 'What is the stock price of Tesla?' for the stock agent and 'What is the weather in San Francisco?' for the search agent.
- src/praisonai-agents/mcp-test.py
- Created a new file to test MCP tool execution with proper error handling.
- Defines a server configuration pointing to a stock price application using
StdioServerParameters. - Implements an asynchronous function
execute_toolto call a tool with error handling. - The
mainfunction initializes a client session, lists available tools, and calls the first available tool with empty parameters for demonstration purposes. - Prints the tool response for verification.
- src/praisonai-agents/praisonaiagents/init.py
- Added
MCPto the list of exported classes and functions in the__init__.pyfile to make it accessible from thepraisonaiagentspackage. The first change adds the import statement, and the second change addsMCPto the__all__list.
- Added
- src/praisonai-agents/praisonaiagents/mcp/mcp.py
- Created a new file implementing the
MCPclass for Model Context Protocol integration. - The
MCPclass manages connections to MCP servers, executes tools, and generates tool functions. - It uses a dedicated thread (
MCPToolRunner) to handle MCP operations asynchronously. - The class supports initializing the MCP connection using either a command and arguments separately or a single command string.
- It dynamically generates wrapper functions for each MCP tool, creating a user-friendly interface for agents to call these tools.
- The class also handles the cleanup of resources when the object is garbage collected.
- Created a new file implementing the
- src/praisonai-agents/pyproject.toml
- Incremented the package version from 0.0.65 to 0.0.66.
- src/praisonai-agents/uv.lock
- Updated the package version from 0.0.65 to 0.0.66.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
From agents keen, with tasks in sight,
MCP emerges, shining bright.
Connecting tools, a seamless flow,
To fetch the data, help them grow.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces MCP integration for stock price retrieval, which is a valuable addition to the praisonai-agents library. The changes include new files for MCP agent functionality, an MCP class for managing connections, and example agents for stock price checking and web searching. Overall, the code is well-structured, but there are some areas that could be improved for clarity, efficiency, and maintainability.
Summary of Findings
- Hardcoded Paths: The code contains several hardcoded paths, such as the path to the Python interpreter and the stock price app. This makes the code less portable and harder to configure. It would be better to use environment variables or configuration files to specify these paths.
- Error Handling in MCPToolRunner: The error handling in the
MCPToolRunnerclass could be improved. Currently, errors during tool execution are caught and returned as strings. It would be better to raise exceptions or use a more structured error reporting mechanism. - Tool Discovery: The tool discovery mechanism in
mcp-basic.pyrelies on string matching (if "stock" in tool.name.lower() or "price" in tool.name.lower():). This is not very robust and could lead to incorrect tool selection. A more reliable method would be to use tool schemas or annotations to identify the correct tool. - Asynchronous Execution: The
stock_price_toolfunction inmcp-basic.pyusesasyncio.runto execute the asynchronousget_stock_pricefunction. This blocks the event loop and can lead to performance issues. It would be better to useasyncio.create_taskor a similar mechanism to run the asynchronous function in a non-blocking manner.
Merge Readiness
The pull request introduces important functionality. However, due to the presence of hardcoded paths and potential improvements in error handling and tool discovery, I recommend addressing the identified issues before merging. I am unable to approve this pull request, and other reviewers should review and approve this code before merging.
| def stock_price_tool(symbol: str) -> str: | ||
| """Get the current stock price for a given symbol""" | ||
| # Run the async function to get the stock price | ||
| result = asyncio.run(get_stock_price(symbol)) | ||
| return f"Stock price for {symbol}: {result}" |
There was a problem hiding this comment.
Using asyncio.run here blocks the event loop. It would be better to use asyncio.create_task or a similar mechanism to run the asynchronous function in a non-blocking manner. Consider refactoring to use await directly within an async context if possible.
# Run the async function to get the stock price
loop = asyncio.get_event_loop()
result = loop.run_until_complete(get_stock_price(symbol))
return f"Stock price for {symbol}: {result}"| command="/Users/praison/miniconda3/envs/mcp/bin/python", | ||
| args=["/Users/praison/stockprice/app.py"] |
There was a problem hiding this comment.
The command and args are hardcoded. Consider using environment variables or a configuration file to make this more configurable and portable.
| command="/Users/praison/miniconda3/envs/mcp/bin/python", | |
| args=["/Users/praison/stockprice/app.py"] | |
| command=os.getenv("MCP_PYTHON_PATH", "/Users/praison/miniconda3/envs/mcp/bin/python"), | |
| args=[os.getenv("STOCKPRICE_APP_PATH", "/Users/praison/stockprice/app.py")] |
| instructions="""You are a helpful assistant that can check stock prices and perform other tasks. | ||
| Use the available tools when relevant to answer user questions.""", | ||
| llm="gpt-4o-mini", | ||
| tools = MCP("/Users/praison/miniconda3/envs/mcp/bin/python /Users/praison/stockprice/app.py") |
There was a problem hiding this comment.
This path is hardcoded. Consider using environment variables or a configuration file to make this more configurable and portable.
| tools = MCP("/Users/praison/miniconda3/envs/mcp/bin/python /Users/praison/stockprice/app.py") | |
| tools = MCP(os.getenv("MCP_COMMAND", "/Users/praison/miniconda3/envs/mcp/bin/python /Users/praison/stockprice/app.py")) |
| command="/Users/praison/miniconda3/envs/mcp/bin/python", | ||
| args=[ | ||
| "/Users/praison/stockprice/app.py", | ||
| ], |
There was a problem hiding this comment.
These paths are hardcoded. Consider using environment variables or a configuration file to make this more configurable and portable.
| command="/Users/praison/miniconda3/envs/mcp/bin/python", | |
| args=[ | |
| "/Users/praison/stockprice/app.py", | |
| ], | |
| command=os.getenv("MCP_PYTHON_PATH", "/Users/praison/miniconda3/envs/mcp/bin/python"), | |
| args=[ | |
| os.getenv("STOCKPRICE_APP_PATH", "/Users/praison/stockprice/app.py"), | |
| ], |
| for tool in tools: | ||
| if "stock" in tool.name.lower() or "price" in tool.name.lower(): | ||
| stock_tool = tool | ||
| break |
There was a problem hiding this comment.
This tool discovery mechanism is not very robust. It relies on string matching, which could lead to incorrect tool selection. A more reliable method would be to use tool schemas or annotations to identify the correct tool.
# Find a tool that can get stock prices based on schema or annotations
stock_tool = next((tool for tool in tools if tool.inputSchema and "ticker" in tool.inputSchema.get("properties", {})), None)
src/praisonai-agents/mcp-test.py
Outdated
| command="/Users/praison/miniconda3/envs/mcp/bin/python", | ||
| args=[ | ||
| "/Users/praison/stockprice/app.py", | ||
| ], |
There was a problem hiding this comment.
These paths are hardcoded. Consider using environment variables or a configuration file to make this more configurable and portable.
| command="/Users/praison/miniconda3/envs/mcp/bin/python", | |
| args=[ | |
| "/Users/praison/stockprice/app.py", | |
| ], | |
| command=os.getenv("MCP_PYTHON_PATH", "/Users/praison/miniconda3/envs/mcp/bin/python"), | |
| args=[ | |
| os.getenv("STOCKPRICE_APP_PATH", "/Users/praison/stockprice/app.py"), | |
| ], |
| except Exception as e: | ||
| self.result_queue.put((False, str(e))) |
There was a problem hiding this comment.
Consider raising an exception with more context instead of putting a string in the queue. This will allow for better error handling and debugging.
| except Exception as e: | |
| self.result_queue.put((False, str(e))) | |
| except Exception as e: | |
| self.result_queue.put((False, e)) |
| break | ||
| except Exception as e: | ||
| self.initialized.set() # Ensure we don't hang | ||
| self.result_queue.put((False, f"MCP initialization error: {str(e)}")) |
There was a problem hiding this comment.
Consider raising an exception with more context instead of putting a string in the queue. This will allow for better error handling and debugging.
| self.result_queue.put((False, f"MCP initialization error: {str(e)}")) | |
| self.result_queue.put((False, e)) |
| if not self.initialized.is_set(): | ||
| self.initialized.wait(timeout=30) | ||
| if not self.initialized.is_set(): | ||
| return "Error: MCP initialization timed out" |
There was a problem hiding this comment.
Instead of returning a string, consider raising an exception to signal the timeout.
| return "Error: MCP initialization timed out" | |
| raise TimeoutError("MCP initialization timed out") |
| # Wait for result | ||
| success, result = self.result_queue.get() | ||
| if not success: | ||
| return f"Error: {result}" |
There was a problem hiding this comment.
Instead of returning a string, consider raising the exception to signal the error.
| return f"Error: {result}" | |
| raise Exception(result) |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 16046208 | Triggered | Generic High Entropy Secret | 531dd4b | src/praisonai-agents/mcp-npx-brave.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
✅ Deploy Preview for praisonai canceled.
|
![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 (12)
src/praisonai-agents/mcp-agents.py (1)
1-10: Agent setup looks good but contains hardcoded pathsThe code correctly imports and utilizes the new MCP functionality. However, it uses absolute paths which could cause portability issues.
Consider replacing hardcoded paths with environment variables or configuration settings:
-tools = MCP("/Users/praison/miniconda3/envs/mcp/bin/python /Users/praison/stockprice/app.py") +tools = MCP(os.getenv("PYTHON_PATH", "python") + " " + os.getenv("STOCKPRICE_APP_PATH", "./stockprice/app.py"))Don't forget to add the import:
import ossrc/praisonai-agents/mcp-agents-detailed.py (2)
1-13: Agent setup with MCP looks correct but contains hardcoded pathsThe code correctly demonstrates the alternative way to instantiate the MCP class by providing the command and args separately, which is a good example. However, it also contains hardcoded paths that could cause portability issues.
Consider using environment variables or configuration files instead of hardcoded paths:
- tools=MCP( - command="/Users/praison/miniconda3/envs/mcp/bin/python", - args=["/Users/praison/stockprice/app.py"] - ) + tools=MCP( + command=os.getenv("PYTHON_PATH", "python"), + args=[os.getenv("STOCKPRICE_APP_PATH", "./stockprice/app.py")] + )Don't forget to add the import:
import os
3-13: Example code could be improved with better error handlingWhile this example code demonstrates how to use the MCP class, it would be beneficial to add error handling for cases where the MCP server fails to initialize.
Consider adding a try-except block to handle potential exceptions:
+import os from praisonaiagents import Agent, MCP +try: agent = Agent( instructions="""You are a helpful assistant that can check stock prices and perform other tasks. Use the available tools when relevant to answer user questions.""", llm="gpt-4o-mini", tools=MCP( command="/Users/praison/miniconda3/envs/mcp/bin/python", args=["/Users/praison/stockprice/app.py"] ) ) agent.start("What is the stock price of Tesla?") +except Exception as e: + print(f"Error initializing agent: {e}")src/praisonai-agents/mcp-multiagents.py (2)
3-11: Consider making script paths configurable for better portability.Currently, the path to the Python interpreter and the
app.pyscript is hardcoded. If you want this code to run outside your local environment, consider using environment variables or a configuration file to locate the Python binary and script path.- tools=MCP( - command="/Users/praison/miniconda3/envs/mcp/bin/python", - args=["/Users/praison/stockprice/app.py"] - ) + import os + python_path = os.getenv("PYTHON_BIN", "/usr/bin/python") + stock_script = os.getenv("STOCK_SCRIPT_PATH", "/path/to/stockprice/app.py") + tools=MCP( + command=python_path, + args=[stock_script] + )
13-18: Avoid embedding API keys in source code.Placing the Brave API key within the command string can expose sensitive information. Consider using environment variables or a secure secrets manager to protect credentials.
- tools=MCP('npx -y @smithery/cli@latest install @smithery-ai/brave-search --client claude --config "{\"braveApiKey\":\"BSANfDaqLKO9wq7e08mrPth9ZlJvKtc\"}"') + brave_api_key = os.getenv("BRAVE_API_KEY", "REPLACE_ME") + tools=MCP(f'npx -y @smithery/cli@latest install @smithery-ai/brave-search --client claude --config "{{\\"braveApiKey\\":\\"{brave_api_key}\\"}}"')src/praisonai-agents/mcp-test.py (2)
28-31: Consolidate nestedasync withstatements.Ruff (SIM117) suggests using a single
withstatement for multiple contexts. You can reduce indentation and slightly improve readability:- async with stdio_client(server_params) as (read, write): - async with ClientSession(read, write) as session: + async with stdio_client(server_params) as (read, write), ClientSession(read, write) as session:🧰 Tools
🪛 Ruff (0.8.2)
30-31: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
57-59: Refine dictionary creation without calling.keys().Ruff (SIM118) recommends using
tool.inputSchema["properties"]directly to test membership. This is a minor performance and stylistic suggestion:-params = { - key: "" for key in tool.inputSchema["properties"].keys() -} +params = { + key: "" for key in tool.inputSchema["properties"] +}🧰 Tools
🪛 Ruff (0.8.2)
58-58: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
src/praisonai-agents/mcp-basic.py (3)
6-13: Consider making paths portable.Hardcoded paths to your environment and script may limit portability. Use environment variables or config files to store these paths if portability is a concern.
15-26: Singleasync withstatement recommended.You can consolidate multiple
async withblocks into one to follow best practices and linting recommendations.🧰 Tools
🪛 Ruff (0.8.2)
17-18: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
47-51: Watch out for nested event loops inasyncio.run().Calling
asyncio.run(get_stock_price(symbol))may fail if there's already an active event loop (e.g., in a larger async application). For broader usage, consider an approach that checks for or reuses an existing event loop.src/praisonai-agents/praisonaiagents/mcp/mcp.py (2)
4-4: Remove unused imports.Ruff identifies several imports not being used (
time,Dict,Optional,Union,partial). Removing them keeps the code clean and eliminates unnecessary dependencies.-import time -from typing import Dict, Any, List, Optional, Callable, Iterable, Union -from functools import wraps, partial +from typing import Any, List, Callable, Iterable +from functools import wrapsAlso applies to: 7-8
🧰 Tools
🪛 Ruff (0.8.2)
4-4:
timeimported but unusedRemove unused import:
time(F401)
33-34: Consolidate nestedwithstatements.Ruff (SIM117) recommends using a single
withstatement for bothstdio_client(self.server_params)andClientSession(read, write).-async with stdio_client(self.server_params) as (read, write): - async with ClientSession(read, write) as session: +async with stdio_client(self.server_params) as (read, write), ClientSession(read, write) as session:🧰 Tools
🪛 Ruff (0.8.2)
33-34: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/praisonai-agents/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
src/praisonai-agents/mcp-agents-detailed.py(1 hunks)src/praisonai-agents/mcp-agents.py(1 hunks)src/praisonai-agents/mcp-basic.py(1 hunks)src/praisonai-agents/mcp-multiagents.py(1 hunks)src/praisonai-agents/mcp-test.py(1 hunks)src/praisonai-agents/praisonaiagents/__init__.py(2 hunks)src/praisonai-agents/praisonaiagents/mcp/mcp.py(1 hunks)src/praisonai-agents/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
src/praisonai-agents/mcp-agents.py (1)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
MCP(98-278)
src/praisonai-agents/praisonaiagents/__init__.py (1)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
MCP(98-278)
src/praisonai-agents/mcp-multiagents.py (1)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
MCP(98-278)
src/praisonai-agents/mcp-agents-detailed.py (1)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
MCP(98-278)
🪛 Ruff (0.8.2)
src/praisonai-agents/mcp-test.py
30-31: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
58-58: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
src/praisonai-agents/praisonaiagents/mcp/mcp.py
4-4: time imported but unused
Remove unused import: time
(F401)
7-7: typing.Dict imported but unused
Remove unused import
(F401)
7-7: typing.Optional imported but unused
Remove unused import
(F401)
7-7: typing.Union imported but unused
Remove unused import
(F401)
8-8: functools.partial imported but unused
Remove unused import: functools.partial
(F401)
33-34: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
src/praisonai-agents/mcp-basic.py
17-18: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (10)
src/praisonai-agents/pyproject.toml (1)
7-7: Version increment looks goodThe version has been properly incremented from 0.0.65 to 0.0.66, which is appropriate for the addition of the new MCP functionality.
src/praisonai-agents/praisonaiagents/__init__.py (2)
13-13: Import of MCP class looks goodThe MCP class is correctly imported from the mcp module.
55-56: Correctly added MCP to exportsMCP has been appropriately added to the
__all__list, making it available when usingfrom praisonaiagents import *.src/praisonai-agents/mcp-multiagents.py (2)
1-2: Imports look good.The import statement correctly references the required classes, and no issues are apparent with import ordering or code conventions.
20-21: LGTM!Starting both agents to retrieve the stock price for Tesla and searching for weather looks correct. No further issues in this logic.
src/praisonai-agents/mcp-test.py (2)
1-6: No issues with imports.All necessary modules are imported correctly for async usage, JSON handling, and MCP interactions.
15-26: Execution function is well-structured.Good job providing error handling and returning a dictionary with the error message if an exception occurs. This helps keep your asynchronous calls robust.
src/praisonai-agents/mcp-basic.py (2)
1-5: Imports check out.All imports are relevant for asynchronous operation and agent usage.
54-61: Agent logic is straightforward.Using
stock_price_toolfor the agent tool and then callingstart()with a Tesla query looks correct.src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
275-278: Shutdown logic looks fine.Calling
self.runner.shutdown()in__del__gracefully signals the MCP thread to stop, but be mindful that Python garbage collection timing is not guaranteed. Confirm the thread properly finishes without side effects in your environment.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @MervinPraison. * #429 (comment) The following files were modified: * `src/praisonai-agents/mcp-basic.py` * `src/praisonai-agents/mcp-test.py` * `src/praisonai-agents/praisonaiagents/mcp/mcp.py`
|
Note Generated docstrings for this pull request at #430 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (4)
4-10: Remove unused imports to improve code cleanlinessSeveral imports are not used in the code:
timeon line 4OptionalandUnionfromtypingon line 9partialfromfunctoolson line 10- import time import inspect import shlex import logging import os - from typing import Any, List, Optional, Callable, Iterable, Union + from typing import Any, List, Callable, Iterable - from functools import wraps, partial + from functools import wraps🧰 Tools
🪛 Ruff (0.8.2)
4-4:
timeimported but unusedRemove unused import:
time(F401)
9-9:
typing.Optionalimported but unusedRemove unused import
(F401)
9-9:
typing.Unionimported but unusedRemove unused import
(F401)
10-10:
functools.partialimported but unusedRemove unused import:
functools.partial(F401)
35-36: Simplify nested with statementsYou can combine the nested
withstatements into a single statement with multiple contexts for better readability.- async with stdio_client(self.server_params) as (read, write): - async with ClientSession(read, write) as session: + async with stdio_client(self.server_params) as (read, write), ClientSession(read, write) as session:🧰 Tools
🪛 Ruff (0.8.2)
35-36: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
174-175: Use logging instead of print for warning messagesFor consistency and better control over log output, use the logging system instead of print statements.
- print("Warning: MCP initialization timed out") + logging.warning("MCP initialization timed out")
304-304: Use 'raise ... from e' to preserve the exception contextWhen re-raising exceptions, use the
fromsyntax to preserve the original exception context, which helps with debugging.- raise RuntimeError(f"Failed to initialize NPX MCP tools: {e}") + raise RuntimeError(f"Failed to initialize NPX MCP tools: {e}") from e🧰 Tools
🪛 Ruff (0.8.2)
304-304: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
src/praisonai-agents/mcp-npx-airbnb-agent-direct.py (2)
3-3: Consider using the same MCP class as other filesThis file imports MCP from
npx_mcp_wrapper_main, while other similar files import it directly frompraisonaiagents. For consistency, consider using the same approach across all files.-import npx_mcp_wrapper_main +from praisonaiagents import MCPThen update line 8:
- tools=npx_mcp_wrapper_main.MCP( + tools=MCP(
18-18: Consider making this a reusable function rather than executing it directlyTo make this script more reusable, consider wrapping the execution in a function and only running it when the script is invoked directly.
+def main(): + search_agent.start("I want to book an apartment in Paris for 2 nights. 03/28 - 03/30 for 2 adults") + + +if __name__ == "__main__": + main() -search_agent.start("I want to book an apartment in Paris for 2 nights. 03/28 - 03/30 for 2 adults")src/praisonai-agents/mcp-npx-airbnb.py (2)
16-16: Consider making this a reusable function rather than executing it directlyTo make this script more reusable, consider wrapping the execution in a function and only running it when the script is invoked directly.
+def main(): + search_agent.start("I want to book an apartment in Paris for 2 nights. 03/28 - 03/30 for 2 adults") + + +if __name__ == "__main__": + main() -search_agent.start("I want to book an apartment in Paris for 2 nights. 03/28 - 03/30 for 2 adults")
3-14: Consider code reuse for duplicate agent configurationsThis agent configuration is duplicated across multiple files (mcp-npx-airbnb-agent-direct.py and mcp-npx-airbnb-stockprice.py). Consider extracting this common setup into a shared function or configuration file.
You could create a shared module with a function like:
# In a shared module like airbnb_utils.py def create_airbnb_agent(): return Agent( instructions="""You help book apartments on Airbnb.""", llm="gpt-4o-mini", tools=MCP( command="npx", args=[ "-y", "@openbnb/mcp-server-airbnb", "--ignore-robots-txt", ] ) )Then import and use it in your scripts:
from airbnb_utils import create_airbnb_agent search_agent = create_airbnb_agent()src/praisonai-agents/mcp-npx-airbnb-stockprice.py (1)
25-25: Consider making this a reusable function rather than executing it directlyTo make this script more reusable, consider wrapping the execution in a function and only running it when the script is invoked directly.
+def main(): + search_agent.start("I want to book an apartment in Paris for 2 nights. 03/28 - 03/30 for 2 adults") + agent.start("What is the stock price of Tesla?") + + +if __name__ == "__main__": + main() -agent.start("What is the stock price of Tesla?")src/praisonai-agents/mcp-npx-brave.py (2)
3-22: Add descriptive docstrings for the agent initialization.Currently, the code lacks docstrings describing the purpose and usage of
search_agent. Adding them will improve maintainability and help new contributors quickly understand how to configure and use this agent.
24-24: Consider capturing or validating the startup prompt.The hard-coded prompt "Search more information about Praison AI" may be too narrow or might need dynamic customization in a real application. You could make it a parameter or configuration option for flexibility.
src/praisonai-agents/mcp_client_direct.py (2)
18-29: Provide docstrings for themainfunction.Documenting the script’s usage, expected arguments, and outcomes would greatly assist developers in understanding how to run and integrate this MCP client.
33-60: Consider adding fallback or retry logic for connection failures.Although exceptions are caught, it might be helpful to automatically retry or prompt users to re-enter credentials when a transient failure (e.g., network issue) occurs.
src/praisonai-agents/mcp_airbnb_client_direct.py (1)
1-9: Add docstrings for module-level clarity.Include high-level documentation explaining the module’s purpose (booking through an MCP-based Airbnb server), and detail the steps needed to run it successfully.
src/praisonai-agents/mcp_wrapper.py (2)
30-47: Add return type annotations for better clarity.Although the docstrings are thorough, adding explicit return types (e.g.,
-> ClientSessionforconnect_to_server) can aid type checkers and future maintainers.
81-242: Unit-test theexecute_queryflow with mock/mocked MCP sessions.The logic is complex and includes branching for function calls, error handling, and mock responses. Adding unit tests will help ensure correctness and guard against regressions.
src/praisonai-agents/mcp_airbnb_full_direct.py (2)
22-46: Add a docstring foragent_loopfor clarity.Currently,
agent_loopis missing a docstring. Providing an overview of its usage, input parameters, and return type will improve maintainability.
106-111: Combine nested context managers.Static analysis (Ruff SIM117) suggests merging these nested
withstatements into a single block to enhance readability:async def run(): - async with stdio_client(server_params) as (read, write): - async with ClientSession(read, write) as session: + async with stdio_client(server_params) as (read, write), \ + ClientSession(read, write) as session: ...🧰 Tools
🪛 Ruff (0.8.2)
107-111: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
src/praisonai-agents/npx_mcp_wrapper_main.py (2)
49-142: Document concurrency concerns in_initialize_mcp_tools.Creating and deleting a temporary script in this manner can introduce concurrency challenges (e.g., simultaneous writes, stale scripts) when multiple threads or processes run the wrapper concurrently. Consider adding these caveats to the docstring.
54-63: Handle missingnpxgracefully.If
npxis not installed on the system, the code could fail abruptly. Consider verifyingnpxavailability at startup, logging an error if it's missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/praisonai-agents/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
src/praisonai-agents/mcp-multiagents.py(1 hunks)src/praisonai-agents/mcp-npx-airbnb-agent-direct.py(1 hunks)src/praisonai-agents/mcp-npx-airbnb-stockprice.py(1 hunks)src/praisonai-agents/mcp-npx-airbnb.py(1 hunks)src/praisonai-agents/mcp-npx-brave.py(1 hunks)src/praisonai-agents/mcp_airbnb_client_direct.py(1 hunks)src/praisonai-agents/mcp_airbnb_full_direct.py(1 hunks)src/praisonai-agents/mcp_client_direct.py(1 hunks)src/praisonai-agents/mcp_wrapper.py(1 hunks)src/praisonai-agents/npx_mcp_wrapper_main.py(1 hunks)src/praisonai-agents/praisonaiagents/mcp/__init__.py(1 hunks)src/praisonai-agents/praisonaiagents/mcp/mcp.py(1 hunks)src/praisonai-agents/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/praisonai-agents/praisonaiagents/mcp/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/praisonai-agents/pyproject.toml
- src/praisonai-agents/mcp-multiagents.py
🧰 Additional context used
🧬 Code Definitions (5)
src/praisonai-agents/mcp-npx-airbnb-agent-direct.py (2)
src/praisonai-agents/npx_mcp_wrapper_main.py (1)
MCP(12-262)src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
MCP(100-319)
src/praisonai-agents/mcp_airbnb_full_direct.py (1)
src/praisonai-agents/mcp_airbnb_client_direct.py (1)
run(23-63)
src/praisonai-agents/mcp_airbnb_client_direct.py (2)
src/praisonai-agents/mcp_airbnb_full_direct.py (1)
run(106-117)src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
run(27-29)
src/praisonai-agents/mcp_wrapper.py (1)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
call_tool(73-93)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (3)
src/praisonai-agents/mcp_airbnb_client_direct.py (1)
run(23-63)src/praisonai-agents/mcp_airbnb_full_direct.py (1)
run(106-117)src/praisonai-agents/npx_mcp_wrapper_main.py (3)
MCP(12-262)wrapper(154-219)_create_tool_wrapper(143-225)
🪛 Ruff (0.8.2)
src/praisonai-agents/mcp_airbnb_full_direct.py
107-111: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
src/praisonai-agents/praisonaiagents/mcp/mcp.py
4-4: time imported but unused
Remove unused import: time
(F401)
9-9: typing.Optional imported but unused
Remove unused import
(F401)
9-9: typing.Union imported but unused
Remove unused import
(F401)
10-10: functools.partial imported but unused
Remove unused import: functools.partial
(F401)
35-36: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
304-304: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (12)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (7)
60-61: Consider raising an exception with more context instead of putting a string in the queueThis will allow for better error handling and debugging.
- self.result_queue.put((False, str(e))) + self.result_queue.put((False, e))
71-71: Consider raising an exception with more context instead of putting a string in the queueThis will allow for better error handling and debugging.
- self.result_queue.put((False, f"MCP initialization error: {str(e)}")) + self.result_queue.put((False, e))
78-78: Instead of returning a string, consider raising an exception to signal the timeoutThis provides better error propagation and allows callers to catch specific exceptions.
- return "Error: MCP initialization timed out" + raise TimeoutError("MCP initialization timed out")
86-86: Instead of returning a string, consider raising the exception to signal the errorPropagating exceptions allows for better error handling in the calling code.
- return f"Error: {result}" + raise Exception(result)
100-131: Great docstring with clear examples and explanationsThe class docstring is well-written, providing a clear explanation of the class's purpose and including practical examples of how to use it. This makes the API much more accessible to new users.
195-208: Good implementation of tool function generationThe implementation of
_generate_tool_functionsis clean and properly creates wrapper functions for each MCP tool. The method is well-documented and returns the correct type.
210-285: Well-structured tool wrapper creation with proper signature handlingThe
_create_tool_wrappermethod does an excellent job of:
- Analyzing parameter schemas from the tool definition
- Creating appropriate type annotations
- Building a proper function signature
- Wrapping the template function to preserve metadata
- Handling both positional and keyword arguments
This is a robust implementation that ensures the generated functions behave like native Python functions.
src/praisonai-agents/mcp-npx-airbnb-agent-direct.py (1)
5-16: Agent configuration looks goodThe agent is properly configured with clear instructions, an appropriate LLM model, and correctly formatted MCP tool arguments.
src/praisonai-agents/mcp-npx-airbnb-stockprice.py (1)
18-23: Stock price agent configuration has good instructionsThe agent instructions clearly communicate its purpose and available functionality to the user. This is a good practice for clarity.
src/praisonai-agents/mcp_airbnb_client_direct.py (1)
23-63: Validate or sanitize user input for real scenarios.The current script tests a static prompt for booking. In a production environment, user inputs may require validation (e.g., date ranges or location checks) before sending them to the MCP server.
src/praisonai-agents/mcp_airbnb_full_direct.py (1)
54-103: Verify behavior with multiple function calls.The loop enforces a maximum of five tool calls. Any additional calls beyond that limit won't be processed. Confirm that truncating extra calls is the intended behavior.
src/praisonai-agents/npx_mcp_wrapper_main.py (1)
1-11: Good use of logging configuration.The structured logging approach (level, message format, date format) ensures clearer diagnostics and troubleshooting.
| instructions="""You are a helpful assistant that can check stock prices and perform other tasks. | ||
| Use the available tools when relevant to answer user questions.""", | ||
| llm="gpt-4o-mini", | ||
| tools = MCP("/Users/praison/miniconda3/envs/mcp/bin/python /Users/praison/stockprice/app.py") |
There was a problem hiding this comment.
Remove hardcoded absolute paths
Using absolute paths specific to a user's system will prevent the code from working on other machines. Consider using relative paths or environment variables.
- tools = MCP("/Users/praison/miniconda3/envs/mcp/bin/python /Users/praison/stockprice/app.py")
+ tools = MCP("python", args=["stockprice/app.py"])Alternatively, use environment variables:
import os
...
python_path = os.environ.get("MCP_PYTHON_PATH", "python")
app_path = os.environ.get("STOCKPRICE_APP_PATH", "stockprice/app.py")
tools = MCP(python_path, args=[app_path])| "--client", | ||
| "claude", | ||
| "--config", | ||
| '{"braveApiKey":"BSANfDaqLKO9wq7e08mrPth9ZlJvKtc"}' |
There was a problem hiding this comment.
Store API keys in environment variables or a secure store instead of plain text.
Embedding API credentials in the code poses a security risk. It's safer to reference them from environment variables:
- '{"braveApiKey":"BSANfDaqLKO9wq7e08mrPth9ZlJvKtc"}'
+ f'{{"braveApiKey":"{os.getenv("BRAVE_API_KEY")}"}}'📝 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.
| '{"braveApiKey":"BSANfDaqLKO9wq7e08mrPth9ZlJvKtc"}' | |
| f'{{"braveApiKey":"{os.getenv("BRAVE_API_KEY")}"}}' |
| try: | ||
| # Write the temporary script to a file | ||
| temp_script_path = os.path.join(os.path.dirname(__file__), f"_temp_call_mcp_tool_{tool_name}.py") | ||
| with open(temp_script_path, "w") as f: | ||
| f.write(temp_script) | ||
|
|
||
| # Run the temporary script to call the MCP tool | ||
| cmd = ["python", temp_script_path, self.command] + self.args + [tool_name, json.dumps(kwargs)] | ||
| result = subprocess.run(cmd, capture_output=True, text=True, timeout=self.timeout) | ||
|
|
||
| # Remove the temporary script | ||
| os.remove(temp_script_path) | ||
|
|
||
| if result.returncode != 0: | ||
| logger.error(f"Error calling MCP tool {tool_name}: {result.stderr}") | ||
| return {"error": f"Failed to call MCP tool: {result.stderr}"} | ||
|
|
||
| # Parse the result from the script output | ||
| return json.loads(result.stdout.strip()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve security and concurrency for temporary script creation.
Writing a Python script to disk for each invocation can pose race conditions in multi-tenant or parallel scenarios. An in-memory approach or a unique, ephemeral directory can reduce security risks and avoid collisions.
Would you like me to draft an in-memory approach or ephemeral file handling pattern?
mcp-agents-detailed.py,mcp-agents.py,mcp-basic.py,mcp-multiagents.py, andmcp-test.py.MCPclass for managing connections and tool execution.pyproject.tomlanduv.lockto reflect version increment to 0.0.66.Summary by CodeRabbit