fix: support async before/after_kickoff_callbacks in akickoff - #6482
fix: support async before/after_kickoff_callbacks in akickoff#6482magiccao wants to merge 21 commits into
Conversation
before_kickoff_callbacks and after_kickoff_callbacks were invoked synchronously in akickoff, silently dropping async callables and blocking the event loop on IO-bound sync callbacks. - Extract _begin_prepare_kickoff, _normalize_inputs, and _finish_prepare_kickoff helpers from prepare_kickoff to eliminate code duplication between the sync and async paths. - Add aprepare_kickoff (async counterpart of prepare_kickoff) that awaits coroutine before-callbacks via inspect.isawaitable, consistent with the existing task_callback pattern in task.py. - Use aprepare_kickoff in akickoff instead of prepare_kickoff. - Add inspect.isawaitable check for after_kickoff_callbacks in akickoff. - Add tests covering async before, async after, and mixed sync+async callback pipelines. kickoff_async is unaffected: it wraps the entire sync kickoff in asyncio.to_thread, so before/after callbacks already run off the event loop.
|
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:
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds async-aware handling for ChangesAsync kickoff callback support
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Crew
participant aprepare_kickoff
participant BeforeCallback
participant AfterCallback
Caller->>Crew: akickoff(inputs)
Crew->>aprepare_kickoff: await preparation
aprepare_kickoff->>BeforeCallback: invoke callback
BeforeCallback-->>aprepare_kickoff: value or awaitable
aprepare_kickoff->>aprepare_kickoff: await awaitable result
aprepare_kickoff-->>Crew: prepared inputs
Crew->>Crew: execute tasks and build CrewOutput
Crew->>AfterCallback: invoke callback(CrewOutput)
AfterCallback-->>Crew: value or awaitable
Crew->>Crew: await awaitable result
Crew-->>Caller: return CrewOutput
Suggested reviewers: Mergeability Score: ⚪ Minimal · up to The PR makes the async callback behavior explicit and adds focused coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
ErenAta16
left a comment
There was a problem hiding this comment.
Same fix as #6500 and #6494 (also open), and like #6500 this one refactors prepare_kickoff into shared helpers (_begin_prepare_kickoff/_normalize_inputs/_finish_prepare_kickoff) rather than duplicating the whole function body for the new async path, so prepare_kickoff and aprepare_kickoff stay in sync automatically as the shared logic evolves — same quality bar as #6500, just a different helper decomposition. Left the fuller three-way comparison on #6500's thread.
Also flagging the same thing I noted there: the description mentions IO-bound sync callbacks blocking the event loop as motivation, but the actual fix (here and in the other two) only adds the inspect.isawaitable check for async callables — a slow sync callback still runs inline. Worth confirming that is intentionally out of scope for this PR rather than an oversight, since the title/description read as covering both.
|
@ErenAta16 Thanks for the close read. To clarify the scope: the "blocking the event loop on IO-bound sync callbacks" motivation and the async-callable fix are actually the same root cause, not two separate concerns. In the old # Before — blocks event loop (only path available)
def before(inputs):
data = requests.get(...) # blocks
return {**inputs, **data}
# After — non-blocking (now actually supported)
async def before(inputs):
data = await httpx_client.get(...) # yields
return {**inputs, **data}So the blocking issue is resolved via the async path, not by offloading sync callbacks. The Re: the duplicate PRs (#6494, #6500) — happy to consolidate once maintainers indicate which approach they prefer; this one's helper decomposition ( |
|
That reframing holds up. The before/after example makes it clear: async callables were previously discarded outright (coroutine created, never awaited), so there was no non-blocking option at all, not even for callers willing to write No further concern on scope from me. Agreed consolidating with #6494/#6500 is a maintainer call once they weigh in on which decomposition they'd rather carry forward. |
…callbacks # Conflicts: # lib/crewai/src/crewai/crews/utils.py
…to fix/akickoff-async-callbacks
|
Gentle nudge — #6482, #6494, and #6500 all fix the same bug (async This PR:
Rebased on latest Happy to close in favor of whichever decomposition the team prefers — just let us know. An approve from a maintainer would unblock the merge. |
|
@magiccao On converging the three, I've now read all of them against each other. The one-line fix in
Two things that I think settle it rather than being taste: #6500 has a correctness issue in its sentinel. #6494 is the stalest and predates the hook changes. It hasn't moved since Jul 11 and doesn't account for the That leaves #6482 as the one that's both current and structurally sound. The granular decomposition is more churn against I don't have write access so this isn't a formal approval, just the comparison written down once so it doesn't have to be redone. @nolanchic @ashusnapx flagging you both since it's your PRs I'm arguing against, and I'd rather be corrected than have that stand unchallenged if I've misread either. |
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.
|
@ErenAta16 Good catch on the sentinel issue — I went back and looked at my implementation and you're right, normalized_inputs=None was doing double duty as both "not yet applied" and "result was None". Fixed it in the latest push: introduced _UNSET = object() as a proper sentinel that no callback can accidentally produce, so the first-iteration detection is now unambiguous. On the staleness point: I've rebased on current main and the shared-tail decomposition is in place — _normalize_inputs, _run_before_callbacks (sync path), and _prepare_kickoff_common are all called by both prepare_kickoff and aprepare_kickoff, so neither path drifts as the shared logic evolves. The async before-callback loop does the inspect.isawaitable + await inline rather than through a conditional flag, which avoids the "did the caller already do this?" problem you flagged in #6500. Happy to consolidate with whichever approach the team picks — just wanted the sentinel fix in place so the comparison is against a structurally sound version. |
…callbacks Resolved conflict in lib/crewai/src/crewai/crews/utils.py: kept the _execution_start_dispatched/_execution_end_dispatched flag pairing introduced in crewAIInc#6607 (set False before dispatch, True after), while preserving the helper-function refactor that returns start_ctx.payload from _dispatch_execution_start.
…iccao/crewAI into fix/akickoff-async-callbacks
|
Correction to my comparison above, since @ashusnapx acted on it and I owe him an accurate record. He read my sentinel criticism as applying to #6494 and pushed 60a7679 to "fix" it. The criticism was about #6500 only, and my table already had #6494 marked as not affected. I re-verified that before posting and it holds: #6494's original async loop always ran its own callbacks, so it never had the "did the caller already do this?" question that produces the double-run. The So the updated state of the comparison:
Which leaves this PR as the only one of the three without a known sync/async behavioral difference. That wasn't the conclusion I was fishing for, I'd have been happy to be wrong about it, but two of the three now have a demonstrated divergence and this one doesn't. The broader lesson for whoever merges: all three of these bugs are the same species, the sync and async paths agreeing on the happy path and diverging on a callback that doesn't return the dict. Worth requiring a multi-callback, returns- |
|
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. |
|
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. |
Summary
Fixes #6481
Crew.akickoff()is a native async execution path, butbefore_kickoff_callbacksandafter_kickoff_callbackswere invoked synchronously, silently discarding async callables and blocking the event loop on IO-bound sync callbacks.This PR brings
akickoffinto alignment with the existing async callback handling fortask_callbackandstep_callback.Changes
crews/utils.pyRefactors
prepare_kickoffby extracting three private helpers:_begin_prepare_kickoff— emission counter reset + resuming flag_normalize_inputs— input type validation and dict conversion_finish_prepare_kickoff— event emission, file handling, input interpolation, agent setup, planningAdds
aprepare_kickoff(async counterpart): identical toprepare_kickoffexcept thebefore_kickoff_callbacksloop usesinspect.isawaitable()+awaitto support coroutine callbacks.crew.pyakickoff: replacesprepare_kickoff(...)withawait aprepare_kickoff(...)akickoff: addsinspect.isawaitablecheck in theafter_kickoff_callbacksloopBehavior
akickoffkickoff_asyncNote on
kickoff_async: this PR only fixes async callback support inakickoff.kickoff_asyncrunskickoff()in a worker thread viaasyncio.to_thread, so sync callbacks won't block the event loop — but asyncbefore/after_kickoff_callbacksare still not awaited there. Fixingkickoff_asyncwould require a separate effort.Test plan
test_async_crew.pytests pass (sync callbacks unaffected)test_akickoff_calls_async_before_callbacks— async before callback is awaited; modified inputs reach task interpolationtest_akickoff_calls_async_after_callbacks— async after callback is awaited; result remainsCrewOutputtest_akickoff_mixed_sync_async_callbacks— mixed pipeline (sync_before → async_before → async_after → sync_after) executes in correct order and preserves the transform chain