fix(crew): await async before/after_kickoff_callbacks in akickoff#6500
fix(crew): await async before/after_kickoff_callbacks in akickoff#6500nolanchic wants to merge 4 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds async-aware handling for ChangesAsync kickoff callback support
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 |
ErenAta16
left a comment
There was a problem hiding this comment.
This, #6494, and #6482 are three open PRs fixing the same bug (async before/after_kickoff_callbacks silently discarded in akickoff) — the crew.py hunk is identical across all three (same inspect.isawaitable guard around the after-callback loop, same aprepare_kickoff swap-in). Compared the three crews/utils.py implementations, since that is where they actually diverge:
This one and #6482 both extract shared helpers so prepare_kickoff (sync) and aprepare_kickoff (async) share the common setup/teardown logic (event emission, input interpolation, agent setup, planning) rather than duplicating it — this one via a single _prepare_kickoff_impl taking an optional normalized_inputs override, #6482 via three smaller helpers (_begin_prepare_kickoff/_normalize_inputs/_finish_prepare_kickoff). #6494 instead copy-pastes the full ~80-line body of prepare_kickoff into a new aprepare_kickoff, which works but means the two functions can drift out of sync on a future change to the shared logic.
One honest scope note that applies equally to all three: the descriptions (this PR included) mention IO-bound sync callbacks blocking the event loop as part of the motivation, but none of the three actually route sync callbacks through run_in_executor or similar — they only add the inspect.isawaitable check, so a slow sync callback still runs inline on the event loop same as before. Not a defect in this PR specifically, just flagging that the fix (correctly) narrows to "stop silently dropping async callables" rather than also solving the blocking-event-loop half of the stated problem.
akickoff invoked before/after_kickoff_callbacks without an awaitable check, so async callables were silently dropped: an async after-callback replaced the crew result with an unawaited coroutine, and async before-callbacks never ran because prepare_kickoff is synchronous. - Add aprepare_kickoff (async counterpart of prepare_kickoff) that awaits callbacks returning an awaitable, matching task/step_callback handling. Shared body extracted into _prepare_kickoff_impl to avoid duplication. - In akickoff, await the result of each after_kickoff_callback when it is awaitable. Sync kickoff and kickoff_async (to_thread wrapper) are unchanged. Fixes crewAIInc#6481
99d5a42 to
a76d3c7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/tests/crew/test_async_crew.py (1)
274-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix type hint mismatch in
after_callback.The type hint for
after_callbackdeclares the return type asCrewOutput, but it returnsmarker, which is defined as aTaskOutput. While this works at runtime because_post_kickoffhandles it, it violates static typing and will cause warnings in type checkers like mypy.Consider updating the return type hint to match the returned object.
♻️ Proposed fix
marker = TaskOutput( description="replaced", raw="async callback result", agent="Test Agent", ) - async def after_callback(_result: CrewOutput) -> CrewOutput: + async def after_callback(_result: Any) -> TaskOutput: return marker🤖 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/crew/test_async_crew.py` around lines 274 - 281, Update the after_callback function’s return annotation from CrewOutput to TaskOutput so it matches the marker object returned by the callback and passes static type checking.
🤖 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/tests/crew/test_async_crew.py`:
- Around line 274-281: Update the after_callback function’s return annotation
from CrewOutput to TaskOutput so it matches the marker object returned by the
callback and passes static type checking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58d5a7ed-ba2f-430d-af5a-ec31dd7f4ffa
📒 Files selected for processing (3)
lib/crewai/src/crewai/crew.pylib/crewai/src/crewai/crews/utils.pylib/crewai/tests/crew/test_async_crew.py
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/crewai/src/crewai/crew.py
- lib/crewai/src/crewai/crews/utils.py
CodeRabbit nit on crewAIInc#6500: after_callback was annotated to return CrewOutput but returns a TaskOutput marker. Align the hint with the actual returned object so mypy is satisfied.
|
Agree — this PR narrows to "stop silently dropping async callables", and routing sync callbacks through I trimmed that framing from the description and added an explicit scope note that sync callbacks still run inline, so the description no longer overstates what the change does. |
|
Thanks for trimming the description, that reads accurately now. Came back to look at the Simulated the exact control flow from this branch: So it's async-only, and only for callbacks that don't return the dict. That's not an exotic user error though, mutating the passed dict in place and falling off the end of the function is a natural thing to write, and it happens to work fine on Worth noting the async path is the one place where a double-run is most likely to be visibly wrong, since an async callback re-run through A distinct sentinel avoids the ambiguity entirely: _NOT_APPLIED = object()
def _prepare_kickoff_impl(crew, inputs, input_files, *, normalized_inputs=_NOT_APPLIED):
if normalized_inputs is _NOT_APPLIED:
normalized_inputs = _run_before_kickoff_callbacks(crew, _normalize_inputs(inputs))Then Alternatively, restructuring so the impl never runs callbacks (both Either way it'd be worth a test for the returns- |
_prepare_kickoff_impl used None as the 'callbacks not yet applied' marker while aprepare_kickoff passed the real before-callback result through the same parameter. A before-callback that mutates the dict in place and returns None (natural, and working on sync kickoff today) was mistaken for 'not applied', re-running every before-callback on the async path. The re-run also routed async callbacks through the sync runner, producing the un-awaited-coroutine symptom this PR exists to fix. Replace the None default with a dedicated _NOT_APPLIED sentinel object so None round-trips as a value. prepare_kickoff no longer passes the argument; aprepare_kickoff still applies callbacks and passes the result. Adds two async-path regressions: a before-callback returning None runs exactly once, and a pre-applied None (empty callback list) round-trips.
- _prepare_kickoff_impl returned `Any` because normalized was last assigned from InterceptionContext.payload (typed Any); cast it back to dict | None on return so the type-checker CI job stays green. - Drop the redundant `: object` annotation on _NOT_APPLIED to match the repo's other sentinels (bare = object()). - Clarify the _prepare_kickoff_impl docstring: a pre-applied None no longer triggers a re-run, but a callback that returns None still does not flow its inputs to the crew on either path — returning the dict is required. - Strengthen the returns-None regression: spy _run_before_kickoff_callbacks and assert it is never entered on the async path, so the failure surfaces as the double-run assertion instead of a downstream ValidationError.
|
Good catch — confirmed. The Went with the sentinel: _NOT_APPLIED = object()
def _prepare_kickoff_impl(crew, inputs, input_files, *, normalized_inputs=_NOT_APPLIED):
if normalized_inputs is _NOT_APPLIED:
normalized_inputs = _run_before_kickoff_callbacks(crew, _normalize_inputs(inputs))
I considered the restructure (impl never runs callbacks; both callers apply their own and call a shared tail — what #6482 / #6494 do). It's cleaner in that it removes the flag entirely, but it's a meaningfully larger diff, and you'd just signed off on narrowing this PR's scope to "stop silently dropping async callables", so I kept the sentinel to stay minimal. Happy to switch to the restructure here if you'd rather have one canonical implementation rather than three variants — just say the word. Added the two tests you flagged as missing across all three PRs:
One clarification on the Also cleared a Pushed in |
Description
Crew.akickoff()is a native async path, butbefore_kickoff_callbacksandafter_kickoff_callbackswere invoked without any awaitable check — silently dropping async callables:after_kickoff_callbacks:result = after_callback(result)returned an unawaited coroutine when the callback wasasync, so the crew result was silently replaced with a coroutine object (never executed, never awaited).before_kickoff_callbacks: invoked inside the synchronousprepare_kickoff(), which has no async support — async before-callbacks never ran.This is inconsistent with
task_callbackandstep_callback, which correctly checkinspect.isawaitablein the async path.Changes
crews/utils.py: addasync def aprepare_kickoff(...)— the async counterpart ofprepare_kickoff. It awaits before-callbacks that return an awaitable (sync callbacks run unchanged). The shared body is extracted into_prepare_kickoff_impl(...)so the two entry points stay DRY and behavior is identical apart from callback awaiting.crew.py:akickoffnow callsawait aprepare_kickoff(...)instead ofprepare_kickoff(...), and awaits each after-callback result when it is awaitable.Behavior preserved
kickoff()(sync) — unchanged, still usesprepare_kickoff.kickoff_async()— unchanged; it wraps the synckickoffinasyncio.to_thread, so all callbacks already run off the event loop.akickoff— unchanged (anisawaitablecheck is a no-op for them).Test plan
test_akickoff_awaits_async_after_callbacks— async after-callback is awaited; result stays aCrewOutput(was a coroutine before).test_akickoff_applies_async_after_callback_result— async after-callback's returnedCrewOutputreplaces the result.test_akickoff_awaits_async_before_callbacks— async before-callback is awaited and runs.test_akickoff_applies_async_before_callback_inputs— async before-callback's returned inputs flow into the crew.uv run pytest lib/crewai/tests/crew/test_async_crew.py lib/crewai/tests/test_crew.py lib/crewai/tests/test_checkpoint.py→ all pass (sync kickoff + kickoff_async callbacks unaffected).uv run ruff check lib/→ clean.uv run mypy lib/crewai/src/crewai/crew.py lib/crewai/src/crewai/crews/utils.py→ clean (strict).Fixes #6481.