Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding async support for tools #735

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

tg1482
Copy link

@tg1482 tg1482 commented Jun 5, 2024

Following our discussion here, creating another PR with minimal changes to the codebase for supporting async tooling.

Previous PR here - #550

Added a test which passes for executing a pipeline with an async tool.

Screenshot 2024-06-05 at 10 35 27 AM

Comment on lines +130 to +136
if is_async_tool:
if tool.func:
# async tool defined using BaseTool class from crewai_tools
async_tool_run = tool._run
elif tool.coroutine:
# async tool defined using @tool decorator from langchain
async_tool_run = tool._arun
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently when we define an async function using @tool decorator or BaseTools, they get created differently.

For @tool decorator - if the function is a coroutine, func=None and the function gets added to the coroutine attribute.

However, for BaseTools, thats not the case, it remains part of the func attribute. To absorb this complexity from the user's perspective, we do it this way so that users can still define their async tools using _run when using BaseTools. There is no change from the user's perspective other than just adding a async while defining the tool.

Copy link

This PR is stale because it has been open for 45 days with no activity.

@joaomdmoura
Copy link
Collaborator

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

  1. Effective Async Tool Execution: The implementation allows for seamless execution of async tools, preserving essential error handling mechanisms to ensure robustness.
  2. Support for Multiple Tool Decorators: It accommodates both @tool decorators and BaseTool class instances, promoting versatility within the codebase.
  3. 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

  1. 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)
  2. 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)
  3. 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)
  4. 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
  5. 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!

@pythonbyte
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants