Adding async support for tools
Following our discussion here, creating another PR with minimal changes to the codebase for supporting async tooling.
Previous PR here - https://github.com/joaomdmoura/crewAI/pull/550
Added a test which passes for executing a pipeline with an async tool.
This PR is stale because it has been open for 45 days with no activity.
Disclaimer: This review was made by a crew of AI Agents.
Code Review Comment for Async Tooling Implementation
Overview
The pull request introduces asynchronous tool support in crewAI, specifically modifying tool_usage.py and including test cases in crew_test.py. The changes accommodate both decorator-based (@tool) and BaseTool-based asynchronous implementations, enhancing the framework's flexibility and responsiveness.
Positive Aspects
- Effective Async Tool Execution: The implementation allows for seamless execution of async tools, preserving essential error handling mechanisms to ensure robustness.
-
Support for Multiple Tool Decorators: It accommodates both
@tooldecorators andBaseToolclass instances, promoting versatility within the codebase. - Comprehensive Test Coverage: The tests are well-structured to cover both async implementations, ensuring that functionality is preserved across different execution paths.
Issues and Recommendations
-
Async Tool Method Detection Logic: Current implementation:
tool_method = tool.func or tool.coroutine is_async_tool = asyncio.iscoroutinefunction(tool_method)Recommendation: Introduce explicit error checking to avoid runtime issues:
def _get_tool_method(tool): if hasattr(tool, 'func') and tool.func: return tool.func elif hasattr(tool, 'coroutine') and tool.coroutine: return tool.coroutine raise ValueError("Tool must have either func or coroutine attribute") tool_method = _get_tool_method(tool) is_async_tool = asyncio.iscoroutinefunction(tool_method) -
Async Execution Context: Current usage of
asyncio.run()could create new event loops which might be problematic. Recommendation: Use context-aware execution:async def _execute_async_tool(tool_run, *args, **kwargs): try: loop = asyncio.get_running_loop() except RuntimeError: return await tool_run(*args, **kwargs) return await tool_run(*args, **kwargs) -
Code Duplication in Argument Handling: The async execution code is repeated across files. Recommendation: Extract common logic into a single method to reduce repetition:
def _execute_tool(tool_run, is_async, *args, **kwargs): if is_async: return asyncio.run(tool_run(*args, **kwargs)) return tool_run(*args, **kwargs) -
Test Isolation and Performance: Improve test isolation with setup/teardown strategies, and refine test performance by reducing
asyncio.sleep()timeframes. Recommendation: Use fixtures and shorter sleep calls or mocks:@pytest.fixture def async_tool_setup(): async def async_search(query: str) -> str: await asyncio.sleep(0.01) # Reduced to enhance speed return "The capital of France is Paris." return async_search -
Documentation and Type Hints: Provide detailed docstrings and type hints for better maintenance. Recommendation: Include comprehensive type hints and clear documentation:
from typing import Union, Callable, Awaitable ToolResult = Union[str, dict, list] ToolCallable = Union[Callable[..., ToolResult], Callable[..., Awaitable[ToolResult]]]
Links to Related PRs
While specific historical context cannot be provided, reviewing PRs that previously addressed error handling and async functionality can inform improvements in the current implementation.
Conclusion
The changes in this pull request proficiently introduce async capabilities, but addressing suggested areas for improvement will further enhance code quality and maintainability. Streamlining the execution patterns, reinforcing error handling logic, and optimizing tests will make the project more robust and efficient. Thank you for the hard work put into this implementation!
Hey @tg1482 , awesome job on the PR!
Could you take a moment to check if the code you added is still relevant and working? We're here to help with anything you need to update and merge this addition.
Thanks
This PR is stale because it has been open for 45 days with no activity.