Fix: Support async before/after_kickoff_callbacks in akickoff (#6481)#6547
Fix: Support async before/after_kickoff_callbacks in akickoff (#6481)#6547mukktinaadh wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesAsync kickoff flow
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/crewai/src/crewai/crews/utils.py (2)
529-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant local import.
Mappingis already available in the module scope (as seen inprepare_kickoff), so this local import is unnecessary.♻️ Proposed fix
- from collections.abc import Mapping🤖 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/crews/utils.py` at line 529, Remove the redundant local Mapping import in the affected utility function, reusing the module-scope Mapping import already used by prepare_kickoff; leave the surrounding logic unchanged.
539-542: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEvaluate the result of the callback for awaitability instead of using
iscoroutinefunction.Checking if the callback itself is a coroutine function is fragile because it misses callables that return awaitables, such as class instances with an async
__call__method,functools.partialwrapped async functions, or mocked async functions in tests (e.g.,AsyncMock). A more robust pattern is to execute the callback and then check if its returned result is awaitable. This approach directly aligns with the PR objective to "detect awaitable results".
lib/crewai/src/crewai/crews/utils.py#L539-L542: Executebefore_callback(normalized), then useinspect.isawaitable()on the result to decide whether toawaitit.lib/crewai/src/crewai/crew.py#L1258-L1263: Executeafter_callback(result), then useinspect.isawaitable()on the result to decide whether toawaitit.♻️ Proposed fixes
For
lib/crewai/src/crewai/crews/utils.py:- if inspect.iscoroutinefunction(before_callback): - normalized = await before_callback(normalized) - else: - normalized = before_callback(normalized) + result = before_callback(normalized) + if inspect.isawaitable(result): + normalized = await result + else: + normalized = resultFor
lib/crewai/src/crewai/crew.py:- import inspect - for after_callback in self.after_kickoff_callbacks: - if inspect.iscoroutinefunction(after_callback): - result = await after_callback(result) - else: - result = after_callback(result) + import inspect + for after_callback in self.after_kickoff_callbacks: + callback_result = after_callback(result) + if inspect.isawaitable(callback_result): + result = await callback_result + else: + result = callback_result🤖 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/crews/utils.py` around lines 539 - 542, Replace coroutine-function checks with result-based awaitability detection: in lib/crewai/src/crewai/crews/utils.py lines 539-542, execute before_callback(normalized), then await the result only when inspect.isawaitable() returns true; apply the same change to after_callback(result) in lib/crewai/src/crewai/crew.py lines 1258-1263. Preserve synchronous callback results unchanged.
🤖 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/src/crewai/crews/utils.py`:
- Around line 600-601: Update async aprepare_kickoff to call the module-local
setup_agents function defined in this module instead of importing setup_agents
from crewai.agents.agent_builder.utilities.base_setup. Remove the imported
symbol and preserve the existing setup invocation behavior.
- Around line 561-566: Update the event emission flow around
crewai_event_bus.emit and future.result() so waiting for the concurrent future
does not block the async event loop. Await the future through an
asyncio-compatible mechanism while preserving the existing exception-swallowing
behavior.
---
Nitpick comments:
In `@lib/crewai/src/crewai/crews/utils.py`:
- Line 529: Remove the redundant local Mapping import in the affected utility
function, reusing the module-scope Mapping import already used by
prepare_kickoff; leave the surrounding logic unchanged.
- Around line 539-542: Replace coroutine-function checks with result-based
awaitability detection: in lib/crewai/src/crewai/crews/utils.py lines 539-542,
execute before_callback(normalized), then await the result only when
inspect.isawaitable() returns true; apply the same change to
after_callback(result) in lib/crewai/src/crewai/crew.py lines 1258-1263.
Preserve synchronous callback results unchanged.
🪄 Autofix (Beta)
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: 25b2b1c8-19da-4979-a52f-60970c3cda81
📒 Files selected for processing (2)
lib/crewai/src/crewai/crew.pylib/crewai/src/crewai/crews/utils.py
| future = crewai_event_bus.emit(crew, started_event) | ||
| if future is not None: | ||
| try: | ||
| future.result() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Avoid blocking the event loop with future.result().
Calling .result() on a concurrent.futures.Future synchronously within an async def function blocks the asyncio event loop, potentially freezing other concurrent tasks. Yield control back to the event loop properly by awaiting the future.
⚡ Proposed fix
- future = crewai_event_bus.emit(crew, started_event)
- if future is not None:
- try:
- future.result()
- except Exception:
- pass
+ future = crewai_event_bus.emit(crew, started_event)
+ if future is not None:
+ try:
+ if inspect.isawaitable(future):
+ await future
+ else:
+ import asyncio
+ await asyncio.wrap_future(future)
+ except Exception:
+ pass📝 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.
| future = crewai_event_bus.emit(crew, started_event) | |
| if future is not None: | |
| try: | |
| future.result() | |
| except Exception: | |
| pass | |
| future = crewai_event_bus.emit(crew, started_event) | |
| if future is not None: | |
| try: | |
| if inspect.isawaitable(future): | |
| await future | |
| else: | |
| import asyncio | |
| await asyncio.wrap_future(future) | |
| except Exception: | |
| pass |
🤖 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/crews/utils.py` around lines 561 - 566, Update the
event emission flow around crewai_event_bus.emit and future.result() so waiting
for the concurrent future does not block the async event loop. Await the future
through an asyncio-compatible mechanism while preserving the existing
exception-swallowing behavior.
| from crewai.agents.agent_builder.utilities.base_setup import setup_agents | ||
| setup_agents( |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the local setup_agents function.
The async aprepare_kickoff imports setup_agents from crewai.agents.agent_builder.utilities.base_setup, whereas the synchronous prepare_kickoff uses the setup_agents function defined locally in this same module (at line 71). To maintain consistency and avoid potential import circularity or unexpected behavior differences, use the local function.
♻️ Proposed fix
- from crewai.agents.agent_builder.utilities.base_setup import setup_agents
- setup_agents(
+ setup_agents(📝 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.
| from crewai.agents.agent_builder.utilities.base_setup import setup_agents | |
| setup_agents( | |
| setup_agents( |
🤖 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/crews/utils.py` around lines 600 - 601, Update async
aprepare_kickoff to call the module-local setup_agents function defined in this
module instead of importing setup_agents from
crewai.agents.agent_builder.utilities.base_setup. Remove the imported symbol and
preserve the existing setup invocation behavior.
ErenAta16
left a comment
ErenAta16
left a comment
There was a problem hiding this comment.
Checked this against the issue's own proposed fix pattern (and against how task_callback already handles this in the sync-task async path). One gap: both here and in aprepare_kickoff, the async-detection uses inspect.iscoroutinefunction(callback) checked before calling it, rather than inspect.isawaitable(result) checked after calling it (which is what the issue itself proposed, and what task_callback's existing code already does).
iscoroutinefunction only recognizes plain async def functions/methods. It returns False for a callable class instance with an async __call__, or for other general callables that return a coroutine without themselves being coroutine functions. SerializableCallable (the type alias before/after_kickoff_callbacks are typed as) is Callable[..., Any], so callable-class instances are a supported, type-checked use case here, not a hypothetical. For one of those, this fix's iscoroutinefunction check is False, so the callback is invoked synchronously, result becomes an unawaited coroutine, and it silently reproduces the exact original bug for that callback style.
Checking inspect.isawaitable(result) after the call instead (call first, then decide whether to await) would cover both plain async functions and callable-class instances, since it inspects the actual return value rather than trying to classify the callable in advance. Worth fixing before merge given the type hint explicitly allows the case this would miss.
2 participants
Closes #6481. Added
aprepare_kickoffto properly await asyncbefore_kickoff_callbacks, and modifiedakickoffto await asyncafter_kickoff_callbackswhen using native async execution.