feat: Move libsy::algorithm::run to libsy-llm-client. - #324
Open
grahamking wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughLibsy now delegates model-call serving to hosts through ChangesHost-driven model routing
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
crates/libsy-llm-client/Cargo.toml (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the observability dependency versions in the workspace.
Add the OpenTelemetry and tracing declarations to root
[workspace.dependencies], then inherit them in this crate and the other crates that use them. Keep crate-specific features local.tracing-opentelemetry0.33 targetsopentelemetry0.32.🤖 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-llm-client/Cargo.toml` around lines 22 - 35, Move the versions for opentelemetry, opentelemetry_sdk, and tracing-opentelemetry into the root [workspace.dependencies], preserving the compatible opentelemetry 0.32/tracing-opentelemetry 0.33 pairing. Update this crate and every other crate using these dependencies to inherit workspace versions, while keeping crate-specific feature selections local.crates/libsy-llm-client/src/run.rs (1)
91-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider distinguishing a router misconfiguration from a failed model call.
clients.route(&target)returnsLlmClientError::Configurationwhen the router maps models and has no entry for the selected one. Line 116 wraps that inLibsyError::client_call, and Line 118 forwards it to the algorithm throughrespond. The algorithm then treats a host configuration gap the same as an upstream failure and may route around it. For a classifier judge that fails open, this hides the missing mapping instead of surfacing it.This is a deliberate trade-off if fail-open is preferred. If not, return the routing error from
serveso the run aborts with the configuration error.🤖 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-llm-client/src/run.rs` around lines 91 - 119, Update serve so a clients.route(&target) configuration error is returned directly instead of being wrapped by LibsyError::client_call and passed through call.respond. Preserve the existing client-call error wrapping and response flow for successfully routed clients whose model call fails, while ensuring missing model mappings abort the run with the routing configuration error.crates/libsy/src/core/algorithm.rs (2)
313-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm that dropping in-flight calls at
ReturnToAgentis the intended contract, and document it.The loop breaks as soon as
Step::ReturnToAgentarrives. Every future still inin_flightis then dropped without completing. For first-wins hedging this is the desired behavior, and the tests cover it. The doc comment does not state it, so a host author cannot know that aservefuture may be cancelled mid-call. Add one sentence to the doc so hosts do not put non-cancel-safe work (for example, a non-idempotent write or a required cleanup step) insideserve.📝 Proposed doc addition
/// `serve` owns the call: it performs it however the host likes and must fulfill the promise /// with [`CallLlmRequest::respond`]. A failed *model* call belongs in `respond` — the /// algorithm may route around it. Returning `Err` from `serve` aborts the whole run, so /// reserve it for infrastructure failures. Calls are served concurrently, so an algorithm /// that offloads several at once (hedging, fan-out) gets real parallelism. +/// +/// A `serve` future that is still in flight when the algorithm returns its final response is +/// dropped, so `serve` must be cancel-safe.🤖 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/src/core/algorithm.rs` around lines 313 - 369, Update the doc comment for drive to state that when Step::ReturnToAgent is received, any still in-flight serve futures are dropped and may be cancelled before completion. Warn hosts not to place non-cancel-safe operations or required cleanup solely inside serve, while preserving the existing first-wins behavior.
739-745: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
let _ = target;statement.
targetis already consumed at Line 736 to build the decision. Thelet _ = target;line is leftover from the migration away fromcall_llm_targetand adds no effect.♻️ Proposed cleanup
driver.info(ctx.clone(), decision.clone()).await?; - let _ = target; driver🤖 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/src/core/algorithm.rs` around lines 739 - 745, Remove the redundant `let _ = target;` statement before the `driver.call_llm` invocation; `target` is already consumed when constructing `decision`, so leave the existing `RoutedRequest` call unchanged.
🤖 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.
Inline comments:
In `@crates/libsy-llm-client/README.md`:
- Around line 146-165: Add the switchyard_protocol::Decision import to the
README Rust example so the selected_model() call on trace decisions resolves
through the trait. Keep the existing route function and imports unchanged apart
from bringing Decision into scope.
In `@crates/libsy-llm-client/src/observability.rs`:
- Around line 275-309: Update ClientStreamObserver::observe_chunk so handling
LlmResponseChunk::MessageStop records the normal terminal outcome by setting
self.terminal before or alongside recording finish reasons. Preserve the
existing complete() behavior and cancellation handling for streams that end
without MessageStop.
In `@crates/libsy/src/algorithms/llm_class.rs`:
- Around line 1800-1834: Update the test helper Queue::take so an exhausted
queue panics instead of returning "unexpected call", ensuring unexpected model
or judge requests fail the test immediately. Preserve the existing queued
response behavior for available entries and apply the same fix to the
corresponding helper usage around the second referenced section.
In `@crates/libsy/src/algorithms/noop.rs`:
- Around line 97-100: Update the test around the Noop algorithm and drive call
to pass a Serve closure that panics when invoked, replacing echo(). Keep the
test asserting Noop’s synthesized response while ensuring any unexpected
offloaded LLM call fails immediately.
In `@crates/switchyard-server/src/config.rs`:
- Around line 178-198: The router construction paths must reject duplicate keys
instead of allowing ClientRouter to silently overwrite clients: in
crates/switchyard-server/src/config.rs lines 178-198, update build_client_router
to detect repeated target.id values mapped to different llm_client sections and
return an error, and align the startup warning in build at lines 69-75 with this
routing model; in crates/switchyard-py/src/libsy_bindings.rs lines 315-327,
reject duplicate target names in target_cores before collecting them into the
router. Also make the shared ClientRouter contract explicit by adding a fallible
try_new duplicate check or documenting new’s last-write-wins behavior.
---
Nitpick comments:
In `@crates/libsy-llm-client/Cargo.toml`:
- Around line 22-35: Move the versions for opentelemetry, opentelemetry_sdk, and
tracing-opentelemetry into the root [workspace.dependencies], preserving the
compatible opentelemetry 0.32/tracing-opentelemetry 0.33 pairing. Update this
crate and every other crate using these dependencies to inherit workspace
versions, while keeping crate-specific feature selections local.
In `@crates/libsy-llm-client/src/run.rs`:
- Around line 91-119: Update serve so a clients.route(&target) configuration
error is returned directly instead of being wrapped by LibsyError::client_call
and passed through call.respond. Preserve the existing client-call error
wrapping and response flow for successfully routed clients whose model call
fails, while ensuring missing model mappings abort the run with the routing
configuration error.
In `@crates/libsy/src/core/algorithm.rs`:
- Around line 313-369: Update the doc comment for drive to state that when
Step::ReturnToAgent is received, any still in-flight serve futures are dropped
and may be cancelled before completion. Warn hosts not to place non-cancel-safe
operations or required cleanup solely inside serve, while preserving the
existing first-wins behavior.
- Around line 739-745: Remove the redundant `let _ = target;` statement before
the `driver.call_llm` invocation; `target` is already consumed when constructing
`decision`, so leave the existing `RoutedRequest` call unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 899ae0b3-08fb-49c3-b352-5e1d7e4517ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (27)
crates/libsy-llm-client/Cargo.tomlcrates/libsy-llm-client/README.mdcrates/libsy-llm-client/src/lib.rscrates/libsy-llm-client/src/observability.rscrates/libsy-llm-client/src/run.rscrates/libsy-llm-client/tests/observability.rscrates/libsy/Cargo.tomlcrates/libsy/README.mdcrates/libsy/src/algorithms/fall_through.rscrates/libsy/src/algorithms/llm_class.rscrates/libsy/src/algorithms/noop.rscrates/libsy/src/algorithms/passthrough.rscrates/libsy/src/algorithms/rand.rscrates/libsy/src/algorithms/stage.rscrates/libsy/src/algorithms/subagent_affinity_tests.rscrates/libsy/src/algorithms/util/llm_judge.rscrates/libsy/src/core.rscrates/libsy/src/core/algorithm.rscrates/libsy/src/core/testing.rscrates/libsy/src/error.rscrates/libsy/src/lib.rscrates/libsy/src/observability.rscrates/switchyard-py/Cargo.tomlcrates/switchyard-py/src/libsy_bindings.rscrates/switchyard-server/src/config.rscrates/switchyard-server/src/lib.rscrates/switchyard-server/tests/server.rs
💤 Files with no reviewable changes (2)
- crates/libsy/Cargo.toml
- crates/libsy/src/error.rs
grahamking
force-pushed
the
gk-run-to-libsy-llm-client
branch
from
August 6, 2026 20:47
f967193 to
b2c3631
Compare
grahamking
marked this pull request as ready for review
August 6, 2026 20:51
|
grahamking
force-pushed
the
gk-run-to-libsy-llm-client
branch
from
August 7, 2026 13:05
b769a42 to
c74d5e9
Compare
`libsy` previously had two entrypoints: - `run_stream`, the main one - `run`, which wrapped `run_stream` and used `libsy-llm-client` to make actual calls. `run` is a convenience many callers will want. It relies entirely on `libsy-llm-client`. This PR moves `run` into `libsy-llm-client`. That's it. There's now no `default_client` in `libsy`. It never makes any calls. The crate dependency runs `libsy-llm-client` -> `libsy`. The key change in `switchyard-server` is this: - Before: `algorithm.run_observed(..)`. `algorithm` is an instance of `Algorithm` trait. - Now: `switchyard_llm_client::run(..)`. Notice the `::`. The bulk of the PR is moving code so it's large but a small delta. It's mostly mechanical. Internally `libsy` now has a `drive` function that does most of what `run` did (iterating the stream), without the HTTP client calls. `libsy_llm_client::run` calls `drive` with a set of real HTTP clients (`TranslatingLlmClient`). It dispatches the `CallLlm` to the correct client. The leaves the machinery in `libsy::drive`, but the user-facing piece with a fully feature LLM client is in `libsy-llm-client`. Some `libsy` integration tests relied on `libsy-llm-client`, so they now live in `libsy-llm-client`. Observation / metrics that relied on knowing host / port and so on also moved to `libsy-llm-client` (`libsy` doesn't know about these things). To review start with `crates/switchyard-server/src/lib.rs` to see the point of the change. Part of #310 Assisted-by: Claude:Opus 5 medium Signed-off-by: Graham King <grahamk@nvidia.com>
grahamking
force-pushed
the
gk-run-to-libsy-llm-client
branch
from
August 7, 2026 13:55
faae198 to
a88c8d3
Compare
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
1 participant
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.
libsypreviously had two entrypoints:run_stream, the main onerun, which wrappedrun_streamand usedlibsy-llm-clientto make actual calls.runis a convenience many callers will want. It relies entirely onlibsy-llm-client.This PR moves
runintolibsy-llm-client. That's it. There's now nodefault_clientinlibsy. It never makes any calls. The crate dependency runslibsy-llm-client->libsy.The key change in
switchyard-serveris this:algorithm.run_observed(..).algorithmis an instance ofAlgorithmtrait.switchyard_llm_client::run(..). Notice the::.The bulk of the PR is moving code so it's large but a small delta. It's mostly mechanical.
Internally
libsynow has adrivefunction that does most of whatrundid (iterating the stream), without the HTTP client calls.libsy_llm_client::runcallsdrivewith a set of real HTTP clients (TranslatingLlmClient). It dispatches theCallLlmto the correct client. The leaves the machinery inlibsy::drive, but the user-facing piece with a fully feature LLM client is inlibsy-llm-client.Some
libsyintegration tests relied onlibsy-llm-client, so they now live inlibsy-llm-client. Observation / metrics that relied on knowing host / port and so on also moved tolibsy-llm-client(libsydoesn't know about these things).To review start with
crates/switchyard-server/src/lib.rsto see the point of the change.Part of #310
Assisted-by: Claude:Opus 5 medium
Signed-off-by: Graham King grahamk@nvidia.com