fix(examples): forward full ensemble requests and cap concurrent explorations - #202
fix(examples): forward full ensemble requests and cap concurrent explorations#202elyasmnvidian wants to merge 1 commit into
Conversation
08758e1 to
72d0323
Compare
72d0323 to
b545e05
Compare
WalkthroughThe ensemble router now uses reservation-based exploration state with asynchronous completion notifications. It preserves structured requests through candidate and committed paths, aggregates streamed candidate responses for judging, and adds concurrency and failure-handling tests. ChangesEnsemble router
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/libsy/examples/ensemble.rs (1)
313-335: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCandidate streams are drained serially — fold aggregation into the fan-out futures.
join_allonly awaits the call handshake; withstream = truenow forwarded to candidates, the actual token streams are consumed one after another here, so candidate n's stream isn't polled until n-1 is fully drained. Aggregating inside each candidate future keeps the whole fan-out concurrent.♻️ Aggregate each candidate inside its own future
calls.push(async move { - ( - model, - driver - .call_llm_target(ctx, &target, call_request, decision) - .await, - ) + let aggregated = match driver + .call_llm_target(ctx, &target, call_request, decision) + .await + { + Ok(Response { + llm_response, + metadata, + }) => llm_response.into_agg().await.map(|agg| Response { + llm_response: LlmResponse::Agg(agg), + metadata, + }), + Err(error) => Err(error.into()), + }; + (model, aggregated) }); } let results = futures::future::join_all(calls).await; - // Aggregate each successful candidate before judging: a streamed candidate - // must be read to completion so the judge can compare its text, and the - // winner is returned as this buffered aggregate. A candidate whose call or - // stream fails is excluded rather than failing the turn. + // A candidate whose call or stream failed is excluded rather than failing + // the turn; the winner is returned as its buffered aggregate. let mut survivors: Vec<(String, Response)> = Vec::new(); for (model, result) in results { - let Ok(response) = result else { continue }; - let Response { - llm_response, - metadata, - } = response; - if let Ok(agg) = llm_response.into_agg().await { - survivors.push(( - model, - Response { - llm_response: LlmResponse::Agg(agg), - metadata, - }, - )); - } + let Ok(response) = result else { continue }; + survivors.push((model, response)); }Note the error types on both arms need to line up (
Result/LibsyError) — adjust the conversion to whatevercall_llm_targetandinto_aggreturn.🤖 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 `@crates/libsy/examples/ensemble.rs` around lines 313 - 335, Move LlmResponse::into_agg aggregation into each candidate future before join_all in the ensemble fan-out, so stream consumption remains concurrent. Update the call_llm_target future’s success and failure branches to return a consistent Result/LibsyError type, then collect only successful aggregated Responses into survivors while preserving model and metadata.
🤖 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 `@crates/libsy/examples/ensemble.rs`:
- Around line 313-335: Move LlmResponse::into_agg aggregation into each
candidate future before join_all in the ensemble fan-out, so stream consumption
remains concurrent. Update the call_llm_target future’s success and failure
branches to return a consistent Result/LibsyError type, then collect only
successful aggregated Responses into survivors while preserving model and
metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64595890-9090-4864-b996-8a6b28b27915
📒 Files selected for processing (1)
crates/libsy/examples/ensemble.rs
b545e05 to
ad35865
Compare
…orations Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
ad35865 to
328c5bc
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
What the ensemble example does
crates/libsy/examples/ensemble.rsis a runnable demo of a custom router built on libsy'sAlgorithmtrait — the interface a router implements, whererunhandles one request (cargo run --example ensemble). For the first few requests it sends each request to several models at once, asks a separate model to pick the best answer, and then commits to whichever model won most often — every later request goes straight to that one model.Terms used below:
Problem
The candidate models never saw the caller's real request. Each candidate call kept only the last user message as plain text; the caller's system message, tools, tool choice, sampling settings, and
streamflag were dropped before the call. A caller who sent this:What happens
Every candidate call — and the committed route — was rebuilt as
text_request(model, prompt_text(request)), which keeps only the last user message. So no matter what the caller sent, each candidate received just this:Every candidate answered a smaller, different question than the caller asked, and the winning model used the same stripped request. Separately, the exploration counter advanced only after the concurrent candidate calls returned, so two requests that arrived together both explored past
exploration_turns; and a turn that errored never returned its slot, so the next request waited forever for the exploration budget to free up.The fix
Two changes, both in the example:
stream. Becausestreamnow reaches candidates, each candidate's answer is read to the end inside its own future before judging. That does two things: the judge compares the real text instead of empty text, and the candidate streams are read at the same time instead of one after another.exploration_turnsexplorations, and a failed turn can no longer hang the next request.Evidence
The judge scores the candidates' real streamed answers instead of empty text:
exploration_turns = 1: the judge is consulted once, and the second request commits from the finished tally instead of starting a second exploration.This is a correctness fix in an example, not a performance change, so there is no benchmark.
How tested
crates/libsy/examples/ensemble.rs, each test driving the realAlgorithm::run:candidate_calls_preserve_the_full_structured_request/committed_route_preserves_the_full_structured_request— candidates and the committed route receive the caller'sLlmRequestunchanged (messages, tools,tool_choice, sampling,stream).streamed_candidates_are_judged_on_their_real_text— the judge sees both streamed answers; the winner returns as buffered text.a_candidate_whose_stream_fails_is_excluded_not_fatal— a stream that breaks partway excludes that candidate instead of failing the turn.concurrent_requests_do_not_exceed_the_exploration_budget— two concurrent requests run one exploration.a_failed_exploration_frees_the_slot_for_the_next_request— a failed turn returns its slot; the next request finishes within a timeout instead of deadlocking.