fix: support async callables in akickoff before/after callbacks#6494
fix: support async callables in akickoff before/after callbacks#6494ashusnapx wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds asynchronous kickoff preparation in ChangesAsync before/after kickoff callback support
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Crew as Crew.akickoff
participant Utils as aprepare_kickoff
participant BeforeCB as before_kickoff_callbacks
participant AfterCB as after_kickoff_callbacks
Caller->>Crew: await akickoff(inputs)
Crew->>Utils: await aprepare_kickoff(crew, inputs)
Utils->>BeforeCB: invoke callback chain
BeforeCB-->>Utils: result or awaitable
Utils->>Utils: await awaitable result
Utils-->>Crew: normalized inputs
Crew->>AfterCB: invoke callback chain
AfterCB-->>Crew: result or awaitable
Crew->>Crew: await awaitable result
Crew-->>Caller: CrewOutput
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
eb92e41 to
04c4ced
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 362-474: The kickoff preparation logic is duplicated between
prepare_kickoff and aprepare_kickoff, creating drift risk as shared behavior
changes over time. Extract the common control flow into a shared internal helper
used by both functions, and keep only the before_kickoff_callbacks execution
different by injecting a callback runner that either returns directly or awaits
awaitable results. Preserve the existing behavior around crewai_event_bus
emission, store_files, _set_tasks_callbacks, setup_agents, and planning in the
shared helper.
- Around line 423-431: The kickoff planning path is still blocking the event
loop because `aprepare_kickoff` ultimately calls `crew._handle_crew_planning()`
synchronously via `CrewPlanner._handle_crew_planning()` and `execute_sync()`.
Update the async kickoff flow in `utils.py` so planning is offloaded to a thread
or handled through a non-blocking async path before returning from `akickoff()`,
using the existing `aprepare_kickoff`, `crew._handle_crew_planning`, and
`CrewPlanner` hooks to locate the change.
🪄 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: 460ccfbf-0675-415f-a37f-8ca756db70d3
📒 Files selected for processing (3)
lib/crewai/src/crewai/crew.pylib/crewai/src/crewai/crews/utils.pylib/crewai/tests/crew/test_async_crew.py
ErenAta16
left a comment
There was a problem hiding this comment.
Same underlying fix as #6500 and #6482 (also open) — the crew.py change here is identical to both. Where this one differs is crews/utils.py: it copy-pastes the full body of prepare_kickoff into a new aprepare_kickoff rather than factoring out the shared setup/teardown logic, which #6500 and #6482 both do via extracted helpers. Functionally correct either way and the test coverage here (including the test_akickoff_mixed_sync_async_callbacks ordering check) is solid, but the duplication means a future change to the shared kickoff-preparation logic would need to be applied in two places instead of one — worth weighing against the other two when picking which of the three to merge.
Closes crewAIInc#6481 Crew.akickoff() is a native async execution path, but before_kickoff_callbacks and after_kickoff_callbacks were invoked synchronously with no awaitable check. Async callables were silently discarded — their coroutine objects were never awaited. - crew.py: Add inspect.isawaitable check in akickoff after_callback loop - crews/utils.py: Add aprepare_kickoff() async variant that awaits async before_kickoff_callbacks using inspect.isawaitable - crew.py: akickoff() now calls aprepare_kickoff() instead of prepare_kickoff() Follows the same pattern used by task_callback and step_callback. Tests: test_akickoff_awaits_async_after_callback, test_akickoff_awaits_async_before_callback, test_akickoff_mixed_sync_async_callbacks. All 14 async crew tests pass. Lint and mypy clean.
04c4ced to
3cfba59
Compare
|
Refactored - extracted shared logic into _prepare_kickoff_common and _run_before_callbacks. Both prepare_kickoff and aprepare_kickoff now call the shared helpers. No more duplication. |
|
Confirmed - |
|
Thanks for confirming. Ready for maintainer review. |
The previous implementation used `None` as a sentinel to detect whether the before-callback loop had started. But a callback can legitimately return `None`, which would cause the next callback to receive the pre-callback inputs instead of the callback's return value. Introduces `_UNSET = object()` as a proper sentinel that no callback can accidentally produce, eliminating the double-run bug identified in the three-way comparison on crewAIInc#6482.
|
@ashusnapx I think there's been a mix-up, and I'd rather flag it than let it ride: the sentinel problem I described was in #6500, not here. Your original version didn't have it. In my comparison table on #6482 I had #6494 marked "callbacks can double-run: no", which I re-verified before writing: The double-run needs the "has the caller already applied the callbacks?" flag that #6500 uses. Your structure never had that question to answer, since Unfortunately 60a7679 has introduced a real divergence while fixing the non-problem. The Two callbacks, first one mutates in place and returns So Keeping the per-iteration guard alongside the awaiting is enough, no sentinel needed: async def aprepare_kickoff(crew, inputs, input_files=None):
normalized = _normalize_inputs(crew, inputs)
for before_callback in crew.before_kickoff_callbacks:
if normalized is None:
normalized = {}
result = before_callback(normalized)
if inspect.isawaitable(result):
result = await result
normalized = result
return _prepare_kickoff_common(crew, normalized, input_files)That's line-for-line the sync loop with two lines added for awaiting, which also makes it obvious at review time that the two paths agree. On your docstring's reasoning, "the next callback would then receive the pre-callback inputs instead of the callback's return value": that isn't what the sync code does. It passes a fresh Also worth a test either way. None of the three PRs covers a multi-callback chain where an early callback returns Sorry for the confusion on my end, my #6482 comment named #6500 but I can see how it read as applying to all three. |
Summary
Fixes silent discarding of async callables in
Crew.akickoff().Closes #6481
Problem
Crew.akickoff()is a native async execution path, butbefore_kickoff_callbacksandafter_kickoff_callbackswere invoked synchronously with noinspect.isawaitablecheck. Async callables were silently discarded — their coroutine objects were never awaited.Fix
after_kickoff_callbacksinakickoff()inspect.isawaitablecheck +awaitbefore_kickoff_callbacksinakickoff()aprepare_kickoff()async variant that awaits async callbacksFollows the same pattern used by
task_callbackandstep_callbackthroughout the codebase:Files Changed
crew.pyimport inspect, fixed after_callback loop, importsaprepare_kickoffcrews/utils.pyaprepare_kickoff()async variant withinspect.isawaitablefor before_callbackstests/crew/test_async_crew.pyTests