Repository navigation
Run SGLang engine calls on one persistent loop so concurrent agenerate works - #565
Open
abhishekraok wants to merge 1 commit into
Open
abhishekraok wants to merge 1 commit into
abhishekraok wants to merge 1 commit into
Conversation
…e works agenerate ran Engine.generate in a worker thread. Engine.generate drives engine.loop with run_until_complete, so a scaffold's concurrent calls (multi_turn_chat runs a chunk's conversations at once) re-entered that loop and failed with "this event loop is already running". One daemon thread now runs engine.loop for the provider's lifetime. generate and agenerate both submit Engine.async_generate to it, which builds the same GenerateReqInput as Engine.generate. agenerate awaits the request instead of holding a thread, so concurrent calls are in the engine together and batch there. The capacity hint reads internal state on the loop, since Engine.get_server_info also drives the loop itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Written by Claude Code session sglang-concurrent-agenerate
Fixes concurrent
agenerateon the SGLang provider. Scaffolds that run a chunk's conversations at once (multi_turn_chat, and by the same patternbfcl_multi_turn, stirrup, vanillux) failed on SGLang withRuntimeError: This event loop is already running. Until now the only workaround wasbatching.chunk_size=1, which runs one conversation at a time (a 5-itemmulti_iftook 2,030 s).Cause
ageneratewasasyncio.to_thread(self.generate, ...), andgeneratecalledEngine.generate, which drivesengine.loopwithrun_until_complete. SGLang binds its tokenizer manager to that one loop, so a second thread callingEngine.generatewhile the first is waiting re-enters the running loop. The docstring said "sequential engine calls", but nothing serialized them.Fix
sglang-engine-loop) runsengine.loopfor the provider's lifetime, started on the first request.generateandagenerateboth submitEngine.async_generateto that loop. At the pinned SGLang commit it builds the sameGenerateReqInputfrom the same arguments asEngine.generateand awaits the sametokenizer_manager.generate_request. So prompts, chat templating, sampling parameters, logprobs and finish reasons are unchanged: preparation and output conversion are the existing code, split into_prepare_batch/_finish_batch.agenerateawaits the request (asyncio.wrap_future) instead of holding a worker thread, so every concurrent conversation is in the engine at once and the scheduler batches them. Rendering and conversion still run in worker threads, so the harness loop is not blocked.Engine.get_server_infoonce the loop thread runs (that method also drives the loop itself). It now readstokenizer_manager.get_internal_state()on the loop, plusserver_args.dp_size: the only fields it used. The startup capacity read in__init__runs before the thread starts and is unchanged.close()stops the loop thread afterengine.shutdown().No lock: nothing is serialized.
Unchanged
describe_requeststill reportsendpoint: engine.generate, since the engine receives the same request; old and new traces stay identical.ageneratealready drove the loop from worker threads, so a background loop thread changes nothing there.Tests
test_concurrent_agenerate_calls_are_batched_on_one_engine_loopuses a fake engine with SGLang's loop contract. Itsgeneratedrivesloop.run_until_complete. Itsasync_generatemust run on that loop and returns only once all 8 requests are in flight together, so a serializing fix would time out. Onmainthe test fails withRuntimeError: This event loop is already running; on this branch it passes, with peak concurrency 8. It also checks that the sync path works while the loop thread is running, and thatclose()stops the thread.async_generatethat records through the existingengine.generatemock, so the existing assertions keep reading one mock. The capacity-hint tests answer throughget_internal_stateinstead ofget_server_info.ruff check,ruff format --check,ty check src/: pass.tests/providers/test_sglang_*.py: 161 passed.GPU smoke
5-item
multi_ifon holmes at the default batching (chunk_size: "auto", resolved to 64), with the same flags as the run that failed onmain(01M73QVRRCVF159KS3N45CVGV2).main, workaround (01M73RN8TFANNA0XGK8XPEETTV)chunk_size=1, serialSmoke2 passes. Its wall time is not directly comparable with the serial run's, because smoke2 drew no runaway conversation. Without the runaway item, the serial run spent about 390 s on its other four conversations; smoke2 ran all five in 226 s.
Smoke1 ran all five conversations concurrently, with no loop error. Its wall time is close to the slowest item alone (1,641 s in the serial run). It still exited 1 on the failure budget. In one conversation (instance 1), a turn sampled at T=0.6 filled the whole 262,144-token context (bare
multi_ifsets nomax_tokens; the suites'_t10_32kvariants cap it at 32,768). The next turn's input was then 28 tokens over the limit, and SGLang rejected it. The serial run onmainhit the same item's length cap (cap-hit 1/5) but drew a shorter history. Sampling is unseeded, so smoke2 is a fresh draw. That overflow is a property of the item and of unbounded multi-turn generation, not of this change; see Follow-ups.The
Task was destroyed but it is pending!lines at exit come from SGLang's tokenizer-manager tasks and appear in themainrun too.Review
Other-family review (Codex,
exec review --base origin/main): no actionable regressions; 100 provider tests passed in its checkout. It could not exercise a real GPU engine; the smoke above covers that.Follow-ups (not in this PR)
agenerateuses the sameasyncio.to_thread(self.generate, ...)pattern.multi_turn_chatsends a turn whose history already exceeds the context, so one runaway turn fails the whole conversation instead of ending it as capped.🤖 Generated with Claude Code