feat: add to_mcp and example in workforce #2550
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPServer
participant Workforce
Client->>MCPServer: process_task / get_workforce_info / add_worker
MCPServer->>Workforce: Call corresponding method
Workforce-->>MCPServer: Return result/status
MCPServer-->>Client: Respond with result/status
Poem
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
camel/societies/workforce/workforce.py (2)
754-758: Split long error message for better readability.The error message exceeds typical line length limits.
- return { - "status": "error", - "message": "Cannot add workers while workforce is running", # noqa: E501 - } + return { + "status": "error", + "message": ( + "Cannot add workers while workforce is running" + ), + }🧰 Tools
🪛 Pylint (3.3.7)
[warning] 754-754: Access to a protected member _running of a client class
(W0212)
789-793: Apply consistent error message formatting.For consistency with the previous suggestion, format this error message the same way.
- return { - "status": "error", - "message": "Cannot add workers while workforce is running", # noqa: E501 - } + return { + "status": "error", + "message": ( + "Cannot add workers while workforce is running" + ), + }🧰 Tools
🪛 Pylint (3.3.7)
[warning] 789-789: Access to a protected member _running of a client class
(W0212)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
camel/societies/workforce/workforce.py(2 hunks)docs/mintlify/reference/camel.societies.workforce.workforce.mdx(1 hunks)examples/workforce/simple_workforce_mcp.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Review the python code against the google python style guide and point out any mismatches.
**/*.py: Review the python code against the google python style guide and point out any mismatches.
examples/workforce/simple_workforce_mcp.pycamel/societies/workforce/workforce.py
🧬 Code Graph Analysis (1)
examples/workforce/simple_workforce_mcp.py (3)
camel/agents/chat_agent.py (2)
ChatAgent(112-1859)system_message(408-410)camel/messages/base.py (2)
BaseMessage(55-553)make_assistant_message(134-176)camel/societies/workforce/workforce.py (4)
Workforce(51-825)add_single_agent_worker(223-237)add_single_agent_worker(746-775)to_mcp(603-825)
🪛 Pylint (3.3.7)
examples/workforce/simple_workforce_mcp.py
[error] 30-30: Unable to import 'camel.agents'
(E0401)
[error] 31-31: Unable to import 'camel.messages'
(E0401)
[error] 32-32: Unable to import 'camel.societies.workforce'
(E0401)
camel/societies/workforce/workforce.py
[refactor] 603-603: Too many arguments (6/5)
(R0913)
[refactor] 603-603: Too many positional arguments (6/5)
(R0917)
[refactor] 603-603: Too many local variables (16/15)
(R0914)
[convention] 634-634: Import outside toplevel (mcp.server.fastmcp.FastMCP)
(C0415)
[warning] 636-639: Consider explicitly re-raising using 'except ImportError as exc' and 'raise ImportError("The 'mcp' package is required to use the to_mcp method. Install it with 'pip install mcp'.") from exc'
(W0707)
[warning] 682-682: Catching too general exception Exception
(W0718)
[warning] 698-698: Catching too general exception Exception
(W0718)
[warning] 708-708: Access to a protected member _children of a client class
(W0212)
[warning] 709-709: Access to a protected member _running of a client class
(W0212)
[warning] 710-710: Access to a protected member _pending_tasks of a client class
(W0212)
[warning] 713-713: Access to a protected member _task of a client class
(W0212)
[warning] 712-712: Access to a protected member _task of a client class
(W0212)
[warning] 723-723: Access to a protected member _children of a client class
(W0212)
[warning] 738-738: Access to a protected member _children of a client class
(W0212)
[warning] 739-739: Access to a protected member _running of a client class
(W0212)
[warning] 774-774: Catching too general exception Exception
(W0718)
[warning] 754-754: Access to a protected member _running of a client class
(W0212)
[warning] 772-772: Access to a protected member _children of a client class
(W0212)
[refactor] 778-778: Too many arguments (7/5)
(R0913)
[refactor] 778-778: Too many positional arguments (7/5)
(R0917)
[warning] 810-810: Catching too general exception Exception
(W0718)
[warning] 789-789: Access to a protected member _running of a client class
(W0212)
[warning] 808-808: Access to a protected member _children of a client class
(W0212)
🪛 Ruff (0.11.9)
camel/societies/workforce/workforce.py
636-639: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
docs/mintlify/reference/camel.societies.workforce.workforce.mdx (1)
261-289: Documentation looks good!The documentation for the
to_mcpmethod is well-structured and follows the existing documentation format. All parameters are properly documented with their types, defaults, and descriptions.examples/workforce/simple_workforce_mcp.py (1)
1-71: Well-structured example demonstrating the new MCP functionality!The example provides clear instructions for testing and demonstrates proper usage of the
to_mcpmethod. Good use of exception handling for graceful shutdown.🧰 Tools
🪛 Pylint (3.3.7)
[error] 30-30: Unable to import 'camel.agents'
(E0401)
[error] 31-31: Unable to import 'camel.messages'
(E0401)
[error] 32-32: Unable to import 'camel.societies.workforce'
(E0401)
| except Exception as e: | ||
| return {"status": "error", "message": str(e)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add logging to exception handlers for better debugging.
Similar to the earlier suggestion, add logging to these exception handlers to aid in debugging.
For the reset function (line 698):
- except Exception as e:
- return {"status": "error", "message": str(e)}
+ except Exception as e:
+ logger.error(f"Error resetting workforce: {e}", exc_info=True)
+ return {"status": "error", "message": str(e)}For add_single_agent_worker (line 774):
- except Exception as e:
- return {"status": "error", "message": str(e)}
+ except Exception as e:
+ logger.error(f"Error adding single agent worker: {e}", exc_info=True)
+ return {"status": "error", "message": str(e)}For add_role_playing_worker (line 810):
- except Exception as e:
- return {"status": "error", "message": str(e)}
+ except Exception as e:
+ logger.error(f"Error adding role playing worker: {e}", exc_info=True)
+ return {"status": "error", "message": str(e)}Also applies to: 774-775, 810-811
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 698-698: Catching too general exception Exception
(W0718)
🤖 Prompt for AI Agents
In camel/societies/workforce/workforce.py at lines 698-699, 774-775, and
810-811, the exception handlers return error messages without logging the
exceptions. Modify these handlers to log the caught exceptions using the
appropriate logger before returning the error response. This will improve
debugging by recording the error details in the logs.
| try: | ||
| from mcp.server.fastmcp import FastMCP | ||
| except ImportError: | ||
| raise ImportError( | ||
| "The 'mcp' package is required to use the to_mcp method. " | ||
| "Install it with 'pip install mcp'." | ||
| ) |
There was a problem hiding this comment.
Use explicit exception chaining in the import error.
When re-raising exceptions, use from err to preserve the exception chain for better debugging.
- try:
- from mcp.server.fastmcp import FastMCP
- except ImportError:
- raise ImportError(
- "The 'mcp' package is required to use the to_mcp method. "
- "Install it with 'pip install mcp'."
- )
+ try:
+ from mcp.server.fastmcp import FastMCP
+ except ImportError as err:
+ raise ImportError(
+ "The 'mcp' package is required to use the to_mcp method. "
+ "Install it with 'pip install mcp'."
+ ) from err📝 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.
| try: | |
| from mcp.server.fastmcp import FastMCP | |
| except ImportError: | |
| raise ImportError( | |
| "The 'mcp' package is required to use the to_mcp method. " | |
| "Install it with 'pip install mcp'." | |
| ) | |
| try: | |
| from mcp.server.fastmcp import FastMCP | |
| except ImportError as err: | |
| raise ImportError( | |
| "The 'mcp' package is required to use the to_mcp method. " | |
| "Install it with 'pip install mcp'." | |
| ) from err |
🧰 Tools
🪛 Ruff (0.11.9)
636-639: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Pylint (3.3.7)
[convention] 634-634: Import outside toplevel (mcp.server.fastmcp.FastMCP)
(C0415)
[warning] 636-639: Consider explicitly re-raising using 'except ImportError as exc' and 'raise ImportError("The 'mcp' package is required to use the to_mcp method. Install it with 'pip install mcp'.") from exc'
(W0707)
🤖 Prompt for AI Agents
In camel/societies/workforce/workforce.py around lines 633 to 639, the
ImportError is re-raised without explicit exception chaining. Modify the except
block to capture the original ImportError as a variable (e.g., err) and re-raise
the new ImportError using 'from err' to preserve the exception chain for better
debugging.
| except Exception as e: | ||
| return { | ||
| "status": "error", | ||
| "message": str(e), | ||
| "task_id": task.id, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more specific exception handling or logging.
Catching generic Exception can hide unexpected errors. Consider catching specific exceptions or at least logging the full exception details for debugging.
- except Exception as e:
- return {
- "status": "error",
- "message": str(e),
- "task_id": task.id,
- }
+ except Exception as e:
+ logger.error(f"Error processing task {task.id}: {e}", exc_info=True)
+ return {
+ "status": "error",
+ "message": str(e),
+ "task_id": task.id,
+ }📝 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.
| except Exception as e: | |
| return { | |
| "status": "error", | |
| "message": str(e), | |
| "task_id": task.id, | |
| } | |
| except Exception as e: | |
| logger.error(f"Error processing task {task.id}: {e}", exc_info=True) | |
| return { | |
| "status": "error", | |
| "message": str(e), | |
| "task_id": task.id, | |
| } |
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 682-682: Catching too general exception Exception
(W0718)
🤖 Prompt for AI Agents
In camel/societies/workforce/workforce.py around lines 682 to 687, the code
catches a generic Exception which can obscure unexpected errors. Refactor the
exception handling to catch more specific exceptions relevant to the code block
or add logging of the full exception details including stack trace before
returning the error response. This will improve error visibility and aid
debugging.
There was a problem hiding this comment.
thanks @MuggleJinx ! Overall LGTM, left some minor comments
| role_name="Assistant", | ||
| agent_kwargs=None, | ||
| ): | ||
| r"""Add a single agent worker to the workforce.""" |
There was a problem hiding this comment.
docstring missing for arguments, same for other functions
| description, | ||
| assistant_role_name, | ||
| user_role_name, | ||
| chat_turn_limit=3, |
There was a problem hiding this comment.
maybe better by default no limit
There was a problem hiding this comment.
the role_playing_worker's default init is 3..
There was a problem hiding this comment.
thanks @MuggleJinx , 3 is too limited, I adjusted this value to 20
|
Hi @MuggleJinx , great work! Thank you. Just a quick question, do you have any local scripts for the client and server that you used to test this feature locally? I am tring to make it work on my machine but kinda stuck. TIA |
Description
Support workforce.to_mcp() function.
Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!