feat(python): expose libsy run stream - #392
Conversation
|
WalkthroughThe libsy Python bindings now use Rust-owned algorithms with asynchronous streaming. Python receives model calls, routing decisions, and terminal responses, then completes model calls through Changeslibsy streaming API
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (3)
tests/test_libsy_minimal_bindings.py (2)
259-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
ModelCall.failguard paths.
PyModelCall::failhas two guards that no test exercises. It raisesTypeErrorwith "error must derive from BaseException" for a non-exception argument. It raisesLibsyErrorwith "model call has already been completed" for a second completion. The duplicate-completion path is covered forrespondat line 90 but not forfail.Add cases inside a
Step.CallModelbranch:with pytest.raises(TypeError, match="must derive from BaseException"): call.fail("not an exception") call.fail(RuntimeError("boom")) with pytest.raises(LibsyError, match="already been completed"): call.fail(RuntimeError("boom"))🤖 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 `@tests/test_libsy_minimal_bindings.py` around lines 259 - 267, Add coverage for both PyModelCall.fail guards in the relevant Step.CallModel test branch: assert that passing a non-BaseException raises TypeError with the expected message, then complete the call with RuntimeError and assert a second fail raises LibsyError indicating the model call is already completed.
62-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive a clear failure when the test forgets to register a client.
Line 63 indexes
clientsoutside thetryblock. If a test omits a client for the selected target, the helper raises a bareKeyErrorwhile theCallModelis still pending. The stream then drops and the algorithm task aborts, so the reported failure does not name the missing target.Raise an explicit error instead.
♻️ Proposed refactor
case Step.CallModel(call): target = call.decision["selected_model_id"] - client = (clients or {})[target] + client = (clients or {}).get(target) + if client is None: + raise AssertionError(f"no client registered for target {target!r}") try: response = await client.call(call.request) except Exception as error: call.fail(error) else: call.respond(response)🤖 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 `@tests/test_libsy_minimal_bindings.py` around lines 62 - 69, Move the client lookup in the call-handling flow into the existing try block so missing registrations are captured before the CallModel remains pending. Convert a missing target from the clients mapping into an explicit error that identifies the selected target, then continue using call.fail(error) while preserving the existing response path for registered clients.crates/switchyard-py/src/libsy_bindings.rs (1)
164-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing
CallModel::algorithmonModelCall.
PyModelCall::newkeeps onlycall.request.llm_requestand the decision. The upstreamCallModelalso carriesalgorithm, which exists so a host that instruments the calls it serves can attribute its own spans to the algorithm behind them. A Python host cannot read that field now.Add an
algorithmgetter if host-side span attribution is a goal for this API.♻️ Proposed addition
struct PyModelCall { inner: Option<CallModel>, + algorithm: String, request: Py<PyAny>, decision: Py<PyAny>, }/// The name of the algorithm that produced this call. #[getter] fn algorithm(&self) -> &str { &self.algorithm }Update
switchyard_rust/libsy.pyif you add the getter.🤖 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/switchyard-py/src/libsy_bindings.rs` around lines 164 - 181, Expose the upstream CallModel algorithm through PyModelCall so Python hosts can attribute spans to the producing algorithm. Preserve the value in PyModelCall::new, add an algorithm getter returning its name, and update switchyard_rust/libsy.py to expose the corresponding property.
🤖 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 `@examples/libsy.py`:
- Around line 13-22: Update EchoClient to accept the selected model during
construction and have call return that stored model instead of the hardcoded
"echo" value. In the example’s routing flow, instantiate EchoClient with
decision.selected_model_id() so the final response matches the printed routing
decision, following the constructor pattern used by the test EchoClient(model).
In `@switchyard_rust/libsy.py`:
- Around line 19-20: Preserve a deprecation path for the removed public APIs: in
switchyard_rust/libsy.py at lines 19-20, document migration for LlmClient and
Algorithm.run or retain a deprecated LlmClient alias that raises
DeprecationWarning, and apply the same decision in switchyard/libsy/__init__.py
at lines 23-24 so both __all__ export surfaces remain consistent.
---
Nitpick comments:
In `@crates/switchyard-py/src/libsy_bindings.rs`:
- Around line 164-181: Expose the upstream CallModel algorithm through
PyModelCall so Python hosts can attribute spans to the producing algorithm.
Preserve the value in PyModelCall::new, add an algorithm getter returning its
name, and update switchyard_rust/libsy.py to expose the corresponding property.
In `@tests/test_libsy_minimal_bindings.py`:
- Around line 259-267: Add coverage for both PyModelCall.fail guards in the
relevant Step.CallModel test branch: assert that passing a non-BaseException
raises TypeError with the expected message, then complete the call with
RuntimeError and assert a second fail raises LibsyError indicating the model
call is already completed.
- Around line 62-69: Move the client lookup in the call-handling flow into the
existing try block so missing registrations are captured before the CallModel
remains pending. Convert a missing target from the clients mapping into an
explicit error that identifies the selected target, then continue using
call.fail(error) while preserving the existing response path for registered
clients.
🪄 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: d74a9cd8-f85e-4341-9f8e-c171cfd63f94
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (6)
crates/switchyard-py/Cargo.tomlcrates/switchyard-py/src/libsy_bindings.rsexamples/libsy.pyswitchyard/libsy/__init__.pyswitchyard_rust/libsy.pytests/test_libsy_minimal_bindings.py
| class EchoClient: | ||
| """Return its configured model as the completion.""" | ||
|
|
||
| def __init__(self, model: str) -> None: | ||
| self.model = model | ||
| """Return a fixed completion for any selected target.""" | ||
|
|
||
| async def call(self, request: Mapping[str, object]) -> Mapping[str, object]: | ||
| return { | ||
| "model": self.model, | ||
| "model": "echo", | ||
| "outputs": [ | ||
| {"role": "assistant", "content": [{"type": "text", "text": self.model}]} | ||
| {"role": "assistant", "content": [{"type": "text", "text": "Hello"}]} | ||
| ], | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make EchoClient honor the routing decision.
EchoClient.call ignores call.decision and always returns "model": "echo". The example prints a decision that selects "fast" or "quality", then prints a final response whose model is "echo". The two outputs contradict each other.
The upstream CallModel documents that the caller must send the request to decision.selected_model_id(), because request["model"] still holds the original agent-supplied name. The example currently teaches the opposite. Pass the selected model into the client, as tests/test_libsy_minimal_bindings.py does with its EchoClient(model).
♻️ Proposed fix
class EchoClient:
"""Return a fixed completion for any selected target."""
- async def call(self, request: Mapping[str, object]) -> Mapping[str, object]:
+ async def call(
+ self, request: Mapping[str, object], model: str
+ ) -> Mapping[str, object]:
return {
- "model": "echo",
+ "model": model,
"outputs": [
{"role": "assistant", "content": [{"type": "text", "text": "Hello"}]}
],
} case Step.CallModel(call):
- call.respond(await client.call(call.request))
+ model = call.decision["selected_model_id"]
+ call.respond(await client.call(call.request, model))Also applies to: 45-46
🤖 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 `@examples/libsy.py` around lines 13 - 22, Update EchoClient to accept the
selected model during construction and have call return that stored model
instead of the hardcoded "echo" value. In the example’s routing flow,
instantiate EchoClient with decision.selected_model_id() so the final response
matches the printed routing decision, following the constructor pattern used by
the test EchoClient(model).
| "ModelCall", | ||
| "Step", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
LlmClient is removed from the public API without a deprecation plan. Both export declarations dropped the LlmClient symbol, and switchyard_rust/libsy.py also replaced Algorithm.run with run_stream. External code that imports LlmClient or calls Algorithm.run breaks with no deprecation signal.
switchyard_rust/libsy.py#L19-L20: record the removal ofLlmClientand ofAlgorithm.runin a migration note, or keep a deprecatedLlmClientalias that raises aDeprecationWarning.switchyard/libsy/__init__.py#L23-L24: apply the same decision to the package__all__so both export surfaces stay consistent.
As per coding guidelines: "Never do - Remove or rename public API exports without an explicit deprecation plan."
📍 Affects 2 files
switchyard_rust/libsy.py#L19-L20(this comment)switchyard/libsy/__init__.py#L23-L24
🤖 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 `@switchyard_rust/libsy.py` around lines 19 - 20, Preserve a deprecation path
for the removed public APIs: in switchyard_rust/libsy.py at lines 19-20,
document migration for LlmClient and Algorithm.run or retain a deprecated
LlmClient alias that raises DeprecationWarning, and apply the same decision in
switchyard/libsy/__init__.py at lines 23-24 so both __all__ export surfaces
remain consistent.
Source: Coding guidelines
Signed-off-by: nachiketb <nachiketb@nvidia.com>
008ae90 to
5af7a75
Compare
1 participant
What
Expose the redesigned libsy
Algorithm::run_streamcontract directly to Python. Python now consumes PyO3 complex-enumStep.CallModel,Step.Decision, andStep.Donevalues, with a typedDecisionobject and dictionary-based LLM requests and responses.Why
The previous binding retained the deleted managed
runabstraction and hid model calls behind a Python client stored on each target. The new API matches libsy: targets are semantic names, and the stream consumer decides how each model call is served.How
Algorithm.runwith an asyncAlgorithm.run_streamiteratorStepas a PyO3 complex enum andModelCallas a one-shot reply handleDecisionPyO3 objectLlmTargetname-only and remove the PythonLlmClientadapterswitchyard-llm-clientandasync-traitfrom the PyO3 crateexamples/libsy.pyto handle the stream directly with one shared clientPython usage
Decision-only integrations answer intermediate judge or classifier calls, then take the final
answer call without serving it:
into_parts()returns the final decision and request after algorithm prompt rewriting. It marksthe run as deliberately abandoned rather than failed, matching the decision-only contract in
issue #310.
Intentional Python differences
The stream control flow mirrors Rust, while the boundary remains Python-native:
Step.Doneaggregates a streaming Rust response before it crosses into PythonCallModel::respond(Result<Response>)is split intorespond(mapping)andfail(exception)Algorithmis an opaque handle created by the provided factories; Python cannot implement the Rust traitrun_stream; raw request metadata is not exposedWhat to review
ModelCall.respond/ModelCall.failownershipDecisionaccess across stream steps, model calls, andinto_parts()ModelCall.into_parts()behaviorValidation
uv run pytest tests/ -v(136 passed)uv run python examples/libsy.pyuv run ruff check .uv run mypy examples/libsy.py switchyard switchyard_rustcargo test -p switchyard-pycargo clippy -p switchyard-py --all-targets -- -D warningscargo fmt --all --checkSummary by CodeRabbit
New Features
ModelCallandSteptypes for handling model interactions.Changes
run_stream.