fix(llm): avoid asyncio.run crash in Codex complete inside active eve…#6928
fix(llm): avoid asyncio.run crash in Codex complete inside active eve…#6928AkshathaaRk wants to merge 1 commit intoaden-hive:mainfrom
Conversation
…nt loop LiteLLMProvider.complete used asyncio.run on the Codex backend path, which crashes with RuntimeError when called from async contexts (notebooks, FastAPI, running event loops). Fix: route Codex complete through a sync bridge helper. If no running loop exists, use asyncio.run normally. If a running loop exists, execute the coroutine in a worker thread. Tests added: test_complete_codex_backend_in_sync_context and test_complete_codex_backend_in_running_event_loop. Verified with uv run pytest tests/test_litellm_provider.py -k codex_backend -q and uv run pytest tests/test_litellm_provider.py -q.
PR Requirements WarningThis PR does not meet the contribution requirements. Missing: No linked issue found. To fix:
Exception: To bypass this requirement, you can:
Micro-fix requirements (must meet ALL):
Why is this required? See #472 for details. |
github-actions
bot
added
the
pr-requirements-warning
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Fixes LiteLLMProvider.complete() crashing in async contexts when using the Codex backend by avoiding direct asyncio.run() calls from threads with an active event loop.
Changes:
- Route Codex-backend
complete()calls through a sync bridge helper that can execute coroutines even when an event loop is already running. - Add tests covering Codex backend completion from both sync callers and from within a running event loop.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/framework/llm/litellm.py | Replaces asyncio.run(self.acomplete(...)) on the Codex path with a helper that runs the coroutine safely when an event loop is active. |
| core/tests/test_litellm_provider.py | Adds regression tests for Codex backend complete() in sync and async (running loop) contexts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| return asyncio.run(coro_factory()) | ||
|
|
||
| def _run_in_worker() -> Any: | ||
| return asyncio.run(coro_factory()) | ||
|
|
||
| with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor: | ||
| return executor.submit(_run_in_worker).result() |
There was a problem hiding this comment.
When a running event loop is detected, this offloads the coroutine to a worker thread but does not propagate contextvars into that thread. This can drop trace/execution context in logs/headers produced inside acomplete()/stream() (the codebase elsewhere explicitly copies context before running work in a thread). Consider wrapping the worker call with contextvars.copy_context() (e.g., submit ctx.run(...)) so observability context is preserved.
|
Maintainer help needed: I cannot assign myself on issue #6929 due to permission limits. Could someone assign @AkshathaaRk on issue #6929 so this PR can pass the contribution requirement? |
|
Closing PR because the contribution requirements were not resolved within the 24-hour grace period. |
Description
Brief description of the changes in this PR.
Type of Change
Changes Made
Required changes
-Fixes #6929
Testing
Describe the tests you ran to verify your changes:
cd core && pytest tests/)cd core && ruff check .)Checklist
Screenshots (if applicable)
Add screenshots to demonstrate UI changes.
Summary by CodeRabbit
Release Notes