fix(experimental): use async LLM calls in AgentExecutor - #6684
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAgentExecutor now awaits asynchronous LLM responses for parsing and native-tool calls. Sync-only providers use ChangesAsync LLM execution
Sequence Diagram(s)sequenceDiagram
participant AgentExecutor
participant aget_llm_response
participant LLM
participant asyncio_to_thread
AgentExecutor->>aget_llm_response: await LLM request
aget_llm_response->>LLM: await acall()
alt Async call unavailable
LLM-->>aget_llm_response: NotImplementedError
aget_llm_response->>asyncio_to_thread: run call()
asyncio_to_thread->>LLM: execute call()
else Async call available
LLM-->>aget_llm_response: response
end
aget_llm_response-->>AgentExecutor: finalized response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
e61a1d4 to
fcafc13
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
love this pr!! |
lorenzejay
left a comment
There was a problem hiding this comment.
Two async-path concerns worth addressing before merge:
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/src/crewai/utilities/agent_utils.py (1)
601-624: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring doesn't document the new sync-fallback/threading behavior.
The function now silently retries via a worker-thread
llm.call()whenllm.acall()raisesNotImplementedError, and offloads_prepare_llm_call/_validate_and_finalize_llm_responseto threads. This non-obvious control flow isn't mentioned in the docstring (e.g., in aNote:section or theRaises:block).📝 Suggested docstring addition
Raises: Exception: If an error occurs. ValueError: If the response is None or empty. + + Note: + If `llm.acall()` raises `NotImplementedError` (sync-only custom LLMs), + this function falls back to running `llm.call()` in a worker thread via + `asyncio.to_thread` so the caller's event loop is not blocked. """As per coding guidelines, "Document public APIs and complex logic in Python code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/utilities/agent_utils.py` around lines 601 - 624, Update the docstring for the async LLM-call function surrounding the documented arguments and return behavior to describe that NotImplementedError from llm.acall() triggers a worker-thread fallback using llm.call(), and that _prepare_llm_call and _validate_and_finalize_llm_response execute in worker threads. Include this control flow in an appropriate Note or Raises section without changing implementation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/crewai/src/crewai/utilities/agent_utils.py`:
- Around line 601-624: Update the docstring for the async LLM-call function
surrounding the documented arguments and return behavior to describe that
NotImplementedError from llm.acall() triggers a worker-thread fallback using
llm.call(), and that _prepare_llm_call and _validate_and_finalize_llm_response
execute in worker threads. Include this control flow in an appropriate Note or
Raises section without changing implementation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2403b5c9-de12-4514-a8b7-760cbaa9ba78
📒 Files selected for processing (4)
lib/crewai/src/crewai/experimental/agent_executor.pylib/crewai/src/crewai/utilities/agent_utils.pylib/crewai/tests/agents/test_agent_executor.pylib/crewai/tests/agents/test_async_agent_executor.py
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/crewai/src/crewai/experimental/agent_executor.py
- lib/crewai/tests/agents/test_agent_executor.py
Head branch was pushed to by a user without write access
|
Refactored associated unit tests to fix mismatches between test cases and the actual code logic. |
bf60ccb to
985c8e1
Compare
Head branch was pushed to by a user without write access
9ab9a52 to
64f2a11
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/tests/llms/azure/test_azure.py`:
- Around line 364-366: Require the tools argument in both test assertions: in
lib/crewai/tests/llms/azure/test_azure.py lines 364-366 and
lib/crewai/tests/llms/google/test_google.py lines 344-346, replace the
conditional checks with direct assertions that await_args.kwargs["tools"] exists
and is nonempty.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ca84485-070d-46d7-b716-46c7e5ed59f9
📒 Files selected for processing (20)
lib/crewai-files/src/crewai_files/cache/upload_cache.pylib/crewai-files/tests/test_upload_cache.pylib/crewai/src/crewai/experimental/agent_executor.pylib/crewai/src/crewai/utilities/agent_utils.pylib/crewai/tests/agents/test_agent.pylib/crewai/tests/agents/test_agent_executor.pylib/crewai/tests/agents/test_async_agent_executor.pylib/crewai/tests/agents/test_native_tool_calling.pylib/crewai/tests/llms/anthropic/test_anthropic.pylib/crewai/tests/llms/azure/conftest.pylib/crewai/tests/llms/azure/test_azure.pylib/crewai/tests/llms/azure/test_azure_async.pylib/crewai/tests/llms/bedrock/test_bedrock.pylib/crewai/tests/llms/google/test_google.pylib/crewai/tests/llms/openai/test_openai.pylib/crewai/tests/llms/openai/test_tools_reasoning_effort_retry.pylib/crewai/tests/telemetry/test_flow_crew_span_integration.pylib/crewai/tests/test_crew_multimodal.pylib/crewai/tests/test_tool_cache_default.pylib/crewai/tests/tools/test_tool_failure.py
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/crewai/src/crewai/utilities/agent_utils.py
- lib/crewai/tests/agents/test_async_agent_executor.py
- lib/crewai/src/crewai/experimental/agent_executor.py
- lib/crewai/tests/agents/test_agent_executor.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/tests/llms/bedrock/test_bedrock.py (1)
273-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the skipped async argument test with an executable test.
Line 273 skips the test, so it cannot detect regressions in the messages passed to
acall(). Use a deterministic unit-level fixture that does not hang, then remove the skip.As per coding guidelines, “Write unit tests for new functionality, focusing on behavior rather than implementation details.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/llms/bedrock/test_bedrock.py` around lines 273 - 315, Replace the skip on test_bedrock_completion_call_arguments with a deterministic unit-level setup that exercises the mocked BedrockCompletion.acall without invoking a potentially hanging full crew execution. Keep the assertions verifying that acall is awaited and receives messages containing “hello world,” and make the test executable as an async test using the project’s established async test conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/tests/llms/bedrock/test_bedrock.py`:
- Around line 406-408: Update the assertions in the awaited request test to
require the “tools” keyword unconditionally, then verify its value is non-null
and non-empty; remove the surrounding conditional so the test fails when acall()
omits tools.
---
Nitpick comments:
In `@lib/crewai/tests/llms/bedrock/test_bedrock.py`:
- Around line 273-315: Replace the skip on
test_bedrock_completion_call_arguments with a deterministic unit-level setup
that exercises the mocked BedrockCompletion.acall without invoking a potentially
hanging full crew execution. Keep the assertions verifying that acall is awaited
and receives messages containing “hello world,” and make the test executable as
an async test using the project’s established async test conventions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed486e2d-07b8-4eed-8d23-1d83452acb0e
📒 Files selected for processing (20)
lib/crewai-files/src/crewai_files/cache/upload_cache.pylib/crewai-files/tests/test_upload_cache.pylib/crewai/src/crewai/experimental/agent_executor.pylib/crewai/src/crewai/utilities/agent_utils.pylib/crewai/tests/agents/test_agent.pylib/crewai/tests/agents/test_agent_executor.pylib/crewai/tests/agents/test_async_agent_executor.pylib/crewai/tests/agents/test_native_tool_calling.pylib/crewai/tests/llms/anthropic/test_anthropic.pylib/crewai/tests/llms/azure/conftest.pylib/crewai/tests/llms/azure/test_azure.pylib/crewai/tests/llms/azure/test_azure_async.pylib/crewai/tests/llms/bedrock/test_bedrock.pylib/crewai/tests/llms/google/test_google.pylib/crewai/tests/llms/openai/test_openai.pylib/crewai/tests/llms/openai/test_tools_reasoning_effort_retry.pylib/crewai/tests/telemetry/test_flow_crew_span_integration.pylib/crewai/tests/test_crew_multimodal.pylib/crewai/tests/test_tool_cache_default.pylib/crewai/tests/tools/test_tool_failure.py
🚧 Files skipped from review as they are similar to previous changes (19)
- lib/crewai/tests/llms/openai/test_tools_reasoning_effort_retry.py
- lib/crewai-files/tests/test_upload_cache.py
- lib/crewai/tests/agents/test_async_agent_executor.py
- lib/crewai/tests/llms/azure/test_azure_async.py
- lib/crewai/tests/tools/test_tool_failure.py
- lib/crewai/tests/telemetry/test_flow_crew_span_integration.py
- lib/crewai/tests/test_crew_multimodal.py
- lib/crewai/tests/agents/test_agent.py
- lib/crewai/src/crewai/utilities/agent_utils.py
- lib/crewai/tests/llms/google/test_google.py
- lib/crewai/tests/test_tool_cache_default.py
- lib/crewai/tests/agents/test_native_tool_calling.py
- lib/crewai/tests/llms/azure/conftest.py
- lib/crewai-files/src/crewai_files/cache/upload_cache.py
- lib/crewai/src/crewai/experimental/agent_executor.py
- lib/crewai/tests/llms/anthropic/test_anthropic.py
- lib/crewai/tests/llms/openai/test_openai.py
- lib/crewai/tests/agents/test_agent_executor.py
- lib/crewai/tests/llms/azure/test_azure.py
Summary
AgentExecutorReAct and native-tool LLM routers asyncaget_llm_response()andLLM.acall()asyncio.to_thread(LLM.call)Fixes #6683
Root cause
AgentExecutor.ainvoke()usesFlow.kickoff_async(), while both LLM router methods called the synchronousget_llm_response(). Provider requests therefore occupied default-executor workers for their full duration and bypassed provider-native async clients.Moving the routers to
LLM.acall()also exposed two existing compatibility assumptions:LLM.call()or synchronous provider clientsBehavior
Providers with async support now run through
LLM.acall(). Custom providers that raiseNotImplementedErrorfromacall()continue through a worker-threadLLM.call()fallback.Request preparation and response finalization run in worker threads so synchronous hooks and validation do not block the event loop. ReAct parsing, native-tool response handling, structured responses, callbacks, and error routing retain their existing semantics.
The forced-final-answer path used after maximum iterations continues to use the existing synchronous
LLM.call()behavior.Scope
This change covers the main LLM routing paths in
crewai.experimental.AgentExecutor, their provider test fixtures, and the CrewAI Files upload-cache bridge reached by async multimodal execution.Async native tool execution remains covered by #6616 and #6622.
Test plan
728 passed, 13 skipped168 passed, 4 skipped5970 passed, 70 skippedgit diff --check