feat(relay): add Switchyard-owned HTTP dynamic plugin - #270
Conversation
0e3eb27 to
58b3186
Compare
58b3186 to
3dcee4d
Compare
WalkthroughThe pull request adds a native Switchyard NeMo Relay plugin with configuration, routing, translation, asynchronous host integration, streaming support, packaging, and documentation. It also adds HTTP transport limits, redirect rejection, timeout defaults, header redaction, and safer error reporting. ChangesTransport safety
NeMo Relay plugin
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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: 6
🧹 Nitpick comments (12)
crates/switchyard-nemo-relay-plugin/src/config.rs (2)
411-421: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the sensitive-header check beyond the exact-match list.
is_sensitive_target_headeruses a fixed denylist. A provider credential header outside that list, for examplex-provider-tokenoropenai-api-key, still passesvalidate_headersand gets stored as plaintext in Relay configuration. The guard at Line 92 exists to prevent exactly that.Add a substring heuristic so unlisted credential headers also route through
header_env.🔒 Proposed change
fn is_sensitive_target_header(name: &str) -> bool { matches!( name, "authorization" | "cookie" | "x-api-key" | "api-key" | "anthropic-api-key" | "x-goog-api-key" ) || name.contains("api-key") || name.contains("api_key") || name.contains("token") || name.contains("secret") || name.contains("password") }🤖 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-nemo-relay-plugin/src/config.rs` around lines 411 - 421, Update is_sensitive_target_header to retain the existing exact-match denylist and also return true when the header name contains a credential-related substring, such as “api-key” or “token,” so unlisted provider credential headers are routed through header_env by validate_headers instead of stored as plaintext.
700-726: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for an invalid environment-supplied header value.
The tests cover an unset variable and invalid variable names. They do not cover
validate_header(name, &value)at Line 129, which rejects a malformed value read from the environment, for example a value that contains a newline or a control character. That path blocks header injection into the provider request.Add a case that sets a variable to an invalid value and asserts that
prepare()fails.Based on learnings from the coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."
🤖 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-nemo-relay-plugin/src/config.rs` around lines 700 - 726, Add a regression test alongside validation_does_not_resolve_environment_backed_headers and invalid_environment_variable_names_are_rejected_before_resolution that sets the referenced environment variable to a malformed header value, such as one containing a newline or control character, then calls config.prepare() and asserts it returns an error. Keep the existing variable-name validation coverage unchanged and verify the failure identifies the invalid header value.Source: Coding guidelines
crates/switchyard-nemo-relay-plugin/src/translation.rs (2)
60-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the Anthropic JSON-schema rejection.
request_policyis the runtime backstop that stops a JSON-schema response format from reaching ananthropic_messagestarget.config.rsat Line 311 rejects only an Anthropic classifier target at configuration time, so this policy is the sole guard for a routed Anthropic target. No test covers it.Add a case that builds an
LlmRequestwith a JSON-schema response format, then assertsvalidate_target_requestfails forWireFormat::AnthropicMessagesand succeeds forWireFormat::OpenAiChat.As per coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."
🤖 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-nemo-relay-plugin/src/translation.rs` around lines 60 - 68, Add a regression test covering request_policy’s JSON-schema capability restriction: construct an LlmRequest using a JSON-schema response format, assert validate_target_request rejects it for WireFormat::AnthropicMessages, and assert validation succeeds for WireFormat::OpenAiChat.Source: Coding guidelines
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo new modules in this crate have no
//!module comment.client.rsstates its intent at Line 4, buttranslation.rsandruntime.rsdo not. The shared root cause is one missing convention pass over the new modules.
crates/switchyard-nemo-relay-plugin/src/translation.rs#L1-L11: add a//!comment stating that the module adapts Relay request and response bodies to Switchyard protocol types and applies the plugin translation policies.crates/switchyard-nemo-relay-plugin/src/runtime.rs#L1-L19: add a//!comment stating that the module decodes inbound requests, drives the libsy algorithm with retries and a trusted fallback, and encodes buffered or streaming responses back to the host.As per coding guidelines: "For Rust changes, document public items with
///comments and add concise comments for module intent, private helpers with non-obvious behavior, important tests, and complex validation, routing, configuration, async, lifecycle, or concurrency logic."🤖 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-nemo-relay-plugin/src/translation.rs` around lines 1 - 11, Add concise //! module documentation to crates/switchyard-nemo-relay-plugin/src/translation.rs#L1-L11 describing adaptation between Relay request/response bodies and Switchyard protocol types, including application of plugin translation policies. Also document crates/switchyard-nemo-relay-plugin/src/runtime.rs#L1-L19 with its role in decoding inbound requests, driving libsy with retries and a trusted fallback, and encoding buffered or streaming responses; no other changes are needed.Source: Coding guidelines
crates/switchyard-nemo-relay-plugin/src/runtime.rs (2)
365-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
tracinginstead ofeprintln!.This code runs inside a plugin loaded by the Relay host.
eprintln!writes to raw stderr, so the message bypasses the host log pipeline and carries no level or structured fields. The workspace already usestracing, for example the spans incrates/libsy-llm-client/src/client.rs.Replace the call with
tracing::warn!.♻️ Proposed change
if let Err(error) = parent.emit_mark(name, &data, metadata) { - eprintln!("Switchyard could not emit routing mark {name:?}: {error}"); + tracing::warn!(mark = name, %error, "Switchyard could not emit routing mark"); }🤖 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-nemo-relay-plugin/src/runtime.rs` around lines 365 - 370, Update the error handling in mark to replace the raw eprintln! call with tracing::warn!, preserving the existing routing-mark error message and including the name and error fields in the structured log.
565-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the retry and stream helpers.
This module holds the retry, fallback, and streaming state machine, and has one test that covers a pure helper.
libsy_error_retryable,failure_mark_data, andreturned_eventsneed no Relay host and are testable now.Cover at minimum:
libsy_error_retryablereturnstruefor each listed status andfalsefor 400, 401, and 404.returned_eventsrejects an emptyLlmResponse::Streamand preserves the first chunk otherwise.failure_mark_datasetsfailure_kindtohttp,non_http, andalgorithmfor the three branches.These tests would have caught the two defects flagged at Lines 134-197 and Lines 259-266.
As per coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."
🤖 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-nemo-relay-plugin/src/runtime.rs` around lines 565 - 601, Extend the existing tests module with unit tests for the pure helpers libsy_error_retryable, returned_events, and failure_mark_data. Verify retryability for every listed status plus false for 400, 401, and 404; ensure returned_events rejects an empty LlmResponse::Stream and retains the first chunk for a non-empty stream; and assert failure_mark_data produces http, non_http, and algorithm for its three branches without requiring a Relay host.Source: Coding guidelines
crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py (2)
16-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
hashlib.file_digestinstead of the manual chunk loop.The coding guidelines target Python 3.12+.
hashlib.file_digestis available from 3.11 and removes the read loop.♻️ Proposed change
def digest(path: Path) -> str: """Return the lowercase SHA-256 digest for a file.""" - value = hashlib.sha256() - with path.open("rb") as stream: - for chunk in iter(lambda: stream.read(1024 * 1024), b""): - value.update(chunk) - return value.hexdigest() + with path.open("rb") as stream: + return hashlib.file_digest(stream, "sha256").hexdigest()🤖 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-nemo-relay-plugin/scripts/package_bundle.py` around lines 16 - 22, Update the digest function to use hashlib.file_digest with the opened file stream and SHA-256, replacing the manual chunk-reading loop while preserving the lowercase hexadecimal digest returned by hexdigest().Source: Coding guidelines
36-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate
config.schema.jsonbefore the script copies files.The script validates the library and the manifest placeholders before it mutates the output directory. It does not validate
config.schema.json. If that file is missing,shutil.copy2raisesFileNotFoundErrorafter the library copy already ran. The output directory is then partially populated and no longer empty, so a rerun fails the emptiness check.♻️ Proposed change
manifest = (CRATE_ROOT / "relay-plugin.toml").read_text(encoding="utf-8") placeholders = ("<platform-library-file>", "<artifact-sha256>") missing = [placeholder for placeholder in placeholders if placeholder not in manifest] if missing: parser.error(f"plugin manifest is missing placeholders: {', '.join(missing)}") + schema = CRATE_ROOT / "config.schema.json" + if not schema.is_file(): + parser.error(f"plugin configuration schema does not exist: {schema}") + output = args.output.resolve() @@ shutil.copy2(library, artifact) - shutil.copy2(CRATE_ROOT / "config.schema.json", output / "config.schema.json") + shutil.copy2(schema, output / "config.schema.json")🤖 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-nemo-relay-plugin/scripts/package_bundle.py` around lines 36 - 51, Validate the existence and required file condition for config.schema.json before creating or mutating the output directory in the packaging flow. Update the logic around the existing manifest and library validation, using the config.schema.json source path, so missing-file errors are reported through parser.error before shutil.copy2 performs either copy; preserve the existing output-directory checks and copy behavior otherwise.crates/switchyard-nemo-relay-plugin/src/executor.rs (1)
113-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a bounded wait in this test.
receiver.recv()blocks forever if the spawned task never runs. The test then hangs CI instead of failing. The second test already usesrecv_timeout. Apply the same pattern here.♻️ Proposed change
- assert_eq!(receiver.recv().unwrap(), "done"); + assert_eq!( + receiver + .recv_timeout(std::time::Duration::from_secs(5)) + .expect("spawned work must complete"), + "done" + );🤖 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-nemo-relay-plugin/src/executor.rs` around lines 113 - 121, Update executor_runs_buffered_and_spawned_work to replace the unbounded receiver.recv() call with receiver.recv_timeout(), using the same bounded-wait pattern and timeout established by the neighboring test.crates/switchyard-nemo-relay-plugin/src/lib.rs (2)
365-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for an unsupported future version.
The tests cover version 1 and a non-integer version. The
Some(version)branch for any other integer is untested. Add a case forversion = 3and assert the "unsupported Switchyard config version" message. The coding guidelines require regression tests for invalid inputs.🤖 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-nemo-relay-plugin/src/lib.rs` around lines 365 - 401, Add a regression test alongside version_one_service_config_gets_a_migration_error_before_v2_deserialization and version_must_be_an_integer that passes {"version": 3} to parse_config, asserts parsing fails, and verifies the error contains the “unsupported Switchyard config version” message.Source: Coding guidelines
4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd module intent comments to the new modules. The new crate omits module-level comments in two files.
ffi.rsincludes a//!comment; these two do not.
crates/switchyard-nemo-relay-plugin/src/lib.rs#L4-L9: add a crate-level//!comment that states the plugin's purpose and the host ABI it targets.crates/switchyard-nemo-relay-plugin/src/executor.rs#L4-L11: add a//!comment that states why the plugin owns a dedicated Tokio runtime thread.The coding guidelines require concise comments for module intent in
crates/**/*.rs.🤖 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-nemo-relay-plugin/src/lib.rs` around lines 4 - 9, Add concise module-level intent comments at both affected sites: in crates/switchyard-nemo-relay-plugin/src/lib.rs lines 4-9, add a crate-level //! comment describing the plugin’s purpose and targeted host ABI; in crates/switchyard-nemo-relay-plugin/src/executor.rs lines 4-11, add a //! comment explaining why the plugin owns a dedicated Tokio runtime thread.Source: Coding guidelines
crates/switchyard-nemo-relay-plugin/src/ffi.rs (1)
323-337: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the wakeup cost of cancellation polling at high concurrency.
Each in-flight call adds one timer wakeup every 10 ms on the two-worker runtime. With thousands of concurrent calls this becomes a constant background load. The host table exposes no cancellation notification, so polling is reasonable now. Consider a backoff that starts short and grows to a longer interval, or make the interval configurable.
🤖 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-nemo-relay-plugin/src/ffi.rs` around lines 323 - 337, Reduce cancellation polling overhead in wait_for_completion_cancellation and wait_for_stream_cancellation by replacing the fixed CANCELLATION_POLL delay with a short initial interval that backs off to a configurable or bounded maximum. Preserve prompt cancellation detection while preventing thousands of in-flight calls from waking every 10 ms indefinitely.
🤖 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/src/client.rs`:
- Around line 285-306: The response handling tests need coverage for an
oversized successful response. Add a regression test in the existing client test
suite that returns a 2xx response whose body exceeds
MAX_BUFFERED_RESPONSE_BYTES, then assert the request fails with
LlmClientError::InvalidResponse while preserving the existing oversized
error-body tests.
In `@crates/switchyard-nemo-relay-plugin/src/ffi.rs`:
- Around line 368-423: Bound the Internal-status retry loops in push_stream and
reject_stream using the existing timing imports and a shared
MAX_BACKPRESSURE_WAIT duration near the other limits. Stop retrying once the
deadline is reached and return an error/status that lets the callback settle the
stream, while preserving cancellation handling and normal successful or
non-Internal responses.
In `@crates/switchyard-nemo-relay-plugin/src/runtime.rs`:
- Around line 372-392: The emit_decision method currently sends prompt-derived
decision.reasoning through ParentScope::emit_mark; constrain this field before
including it in the routing mark. Prefer truncating reasoning to a fixed maximum
length (or gate it behind an off-by-default configuration flag), while
preserving identifier-only metadata and existing decision fields.
- Around line 92-100: Add exponential backoff before retry iterations in the
routing retry loops, including both non-streaming and streaming paths such as
the visible retry branch and execute_stream. When the upstream failure includes
a Retry-After value, use that delay instead of the calculated backoff; otherwise
apply the existing retry-attempt count to compute an exponentially increasing
sleep before re-driving the request.
- Around line 259-266: Bound the outer event-processing loop in the runtime flow
around committed and retry handling so a pass that ends with committed == false
and no retry arm cannot restart indefinitely. Track whether a retry occurred
during the pass, or otherwise detect that no progress was made, and return an
appropriate error before re-entering the outer loop; preserve normal retry and
successful commitment behavior.
- Around line 134-197: Make the fallback_used binding mutable in the surrounding
request loop, and set it to true in the Err(failure) if !fallback_used arm
immediately before switching to the trusted fallback stream via
fallback_response. Preserve the existing retry and error handling for failures
that occur before fallback activation.
---
Nitpick comments:
In `@crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py`:
- Around line 16-22: Update the digest function to use hashlib.file_digest with
the opened file stream and SHA-256, replacing the manual chunk-reading loop
while preserving the lowercase hexadecimal digest returned by hexdigest().
- Around line 36-51: Validate the existence and required file condition for
config.schema.json before creating or mutating the output directory in the
packaging flow. Update the logic around the existing manifest and library
validation, using the config.schema.json source path, so missing-file errors are
reported through parser.error before shutil.copy2 performs either copy; preserve
the existing output-directory checks and copy behavior otherwise.
In `@crates/switchyard-nemo-relay-plugin/src/config.rs`:
- Around line 411-421: Update is_sensitive_target_header to retain the existing
exact-match denylist and also return true when the header name contains a
credential-related substring, such as “api-key” or “token,” so unlisted provider
credential headers are routed through header_env by validate_headers instead of
stored as plaintext.
- Around line 700-726: Add a regression test alongside
validation_does_not_resolve_environment_backed_headers and
invalid_environment_variable_names_are_rejected_before_resolution that sets the
referenced environment variable to a malformed header value, such as one
containing a newline or control character, then calls config.prepare() and
asserts it returns an error. Keep the existing variable-name validation coverage
unchanged and verify the failure identifies the invalid header value.
In `@crates/switchyard-nemo-relay-plugin/src/executor.rs`:
- Around line 113-121: Update executor_runs_buffered_and_spawned_work to replace
the unbounded receiver.recv() call with receiver.recv_timeout(), using the same
bounded-wait pattern and timeout established by the neighboring test.
In `@crates/switchyard-nemo-relay-plugin/src/ffi.rs`:
- Around line 323-337: Reduce cancellation polling overhead in
wait_for_completion_cancellation and wait_for_stream_cancellation by replacing
the fixed CANCELLATION_POLL delay with a short initial interval that backs off
to a configurable or bounded maximum. Preserve prompt cancellation detection
while preventing thousands of in-flight calls from waking every 10 ms
indefinitely.
In `@crates/switchyard-nemo-relay-plugin/src/lib.rs`:
- Around line 365-401: Add a regression test alongside
version_one_service_config_gets_a_migration_error_before_v2_deserialization and
version_must_be_an_integer that passes {"version": 3} to parse_config, asserts
parsing fails, and verifies the error contains the “unsupported Switchyard
config version” message.
- Around line 4-9: Add concise module-level intent comments at both affected
sites: in crates/switchyard-nemo-relay-plugin/src/lib.rs lines 4-9, add a
crate-level //! comment describing the plugin’s purpose and targeted host ABI;
in crates/switchyard-nemo-relay-plugin/src/executor.rs lines 4-11, add a //!
comment explaining why the plugin owns a dedicated Tokio runtime thread.
In `@crates/switchyard-nemo-relay-plugin/src/runtime.rs`:
- Around line 365-370: Update the error handling in mark to replace the raw
eprintln! call with tracing::warn!, preserving the existing routing-mark error
message and including the name and error fields in the structured log.
- Around line 565-601: Extend the existing tests module with unit tests for the
pure helpers libsy_error_retryable, returned_events, and failure_mark_data.
Verify retryability for every listed status plus false for 400, 401, and 404;
ensure returned_events rejects an empty LlmResponse::Stream and retains the
first chunk for a non-empty stream; and assert failure_mark_data produces http,
non_http, and algorithm for its three branches without requiring a Relay host.
In `@crates/switchyard-nemo-relay-plugin/src/translation.rs`:
- Around line 60-68: Add a regression test covering request_policy’s JSON-schema
capability restriction: construct an LlmRequest using a JSON-schema response
format, assert validate_target_request rejects it for
WireFormat::AnthropicMessages, and assert validation succeeds for
WireFormat::OpenAiChat.
- Around line 1-11: Add concise //! module documentation to
crates/switchyard-nemo-relay-plugin/src/translation.rs#L1-L11 describing
adaptation between Relay request/response bodies and Switchyard protocol types,
including application of plugin translation policies. Also document
crates/switchyard-nemo-relay-plugin/src/runtime.rs#L1-L19 with its role in
decoding inbound requests, driving libsy with retries and a trusted fallback,
and encoding buffered or streaming responses; no other changes are needed.
🪄 Autofix (Beta)
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: 3570ba9b-fc47-4a61-b0ff-3d3a77c017b1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (22)
CHANGELOG.mdCargo.tomlREADME.mdcrates/libsy-llm-client/README.mdcrates/libsy-llm-client/src/backend.rscrates/libsy-llm-client/src/client.rscrates/libsy/src/core/algorithm.rscrates/protocol/src/client.rscrates/switchyard-nemo-relay-plugin/Cargo.tomlcrates/switchyard-nemo-relay-plugin/README.mdcrates/switchyard-nemo-relay-plugin/config.schema.jsoncrates/switchyard-nemo-relay-plugin/relay-plugin.tomlcrates/switchyard-nemo-relay-plugin/scripts/package_bundle.pycrates/switchyard-nemo-relay-plugin/src/client.rscrates/switchyard-nemo-relay-plugin/src/config.rscrates/switchyard-nemo-relay-plugin/src/executor.rscrates/switchyard-nemo-relay-plugin/src/ffi.rscrates/switchyard-nemo-relay-plugin/src/lib.rscrates/switchyard-nemo-relay-plugin/src/runtime.rscrates/switchyard-nemo-relay-plugin/src/translation.rscrates/switchyard-translation/src/helpers.rsdocs/index.md
3dcee4d to
1815a45
Compare
|
Blocking follow-up: typed callback refactor causes worker starvation and drops transport cancellationI validated the existing worker-starvation concern against the current head ( Relay worker starvationThe typed callback refactor in With a 500 ms provider:
A separate four-call test with a 1 second provider delayed unrelated work by about 1.008 seconds on current head versus 0.632 ms on the async parent. The threshold is the Relay worker count, so increasing Provider cancellation regressionCaller cancellation returns promptly, but current-head buffered and streaming requests remain open at the provider and continue until response or timeout. In repeated buffered and streaming tests, the current implementation never closed the provider connection before its delayed response; the async parent closed it promptly every time. That can leave orphaned, potentially billable provider work after Hermes has cancelled the call. I think both behaviors need one fix: use a completion-based async Relay boundary rather than synchronously waiting inside the typed callback. Relay already has the raw async ABI; I opened NVIDIA/NeMo-Relay#716 to track a safe typed wrapper. For this PR, the choices are to retain the earlier raw async implementation or coordinate the typed async SDK surface before landing the synchronous refactor. Nonblocking observability follow-up: physical provider and classifier attempts currently have no nested Relay lifecycle. I filed #299 so that requirement can be decided separately from this blocker. |
afourniernv
left a comment
afourniernv
left a comment
There was a problem hiding this comment.
Requesting changes for the worker-starvation and provider-cancellation regressions documented in my review comment:
The current typed callback implementation synchronously waits on Relay's Tokio workers. Controlled testing reproduced unrelated-work latency at the provider-delay boundary once routed concurrency reached the worker count. It also confirmed that cancelling buffered or streaming calls no longer closes the upstream provider request, unlike the immediate completion-based async parent.
Please retain a completion-based async boundary for this plugin or coordinate the safe typed async SDK work tracked in NVIDIA/NeMo-Relay#716 before landing the synchronous refactor.
|
Stable Relay 0.7.0 is now published. I updated the SDK pin, manifest compatibility floor, README, and Cargo.lock and opened a focused PR directly against this PR's head branch: bbednarski9#1 Validation: all 31 switchyard-nemo-relay-plugin tests pass with nemo-relay-plugin and nemo-relay-types 0.7.0. |
5fd58d7 to
54a936f
Compare
5c84c16 to
2ab971c
Compare
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
2ab971c to
6db7275
Compare
|
Review note for the Relay 0.8 follow-up: This PR currently targets Relay 0.7 and therefore implements asynchronous native middleware through the raw completion-based ABI. Relay #751 adds typed, Future-returning native Rust middleware on an SDK-owned Tokio executor. Once that API is released, a separate Switchyard update should be able to remove most of I would keep that migration separate rather than pin this PR to unpublished Relay code. For the current implementation, I would rank the review burden as follows:
The underlying libsy routing algorithms are already exercised elsewhere. The native plugin boundary is the hardest and highest-risk review surface. Relay #751 should remove or abstract most of item 1 and the raw-ABI portion of item 2, materially reducing the review burden rather than only improving syntax. |
|
Current status:
|
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
LOC snapshotPoint-in-time breakdown for PR #270 at
Rust LOC by module:
The remaining 756 lines are 412 lines of documentation/changelog, 282 lines of schema/manifests/Cargo metadata, and 62 lines in the packaging script. |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Alternative design: Relay 0.8 typed async middlewareNVIDIA/NeMo-Relay#751 is now merged and provides the typed asynchronous native middleware surface that was unavailable when this implementation was written. Adopting Relay 0.8 would let this plugin remove both
The managed streaming implementation would retain a small internal bridge because Marks must be emitted while the returned stream is being polled, rather than from the spawned producer task, because Relay restores scope context around the registered future and stream but does not automatically propagate it into arbitrary Expected simplification:
Compatibility changes would be limited to:
Validation should cover managed and unmanaged buffered/streaming calls, routing-mark parentage, cancellation and producer shutdown, and loading the packaged plugin through Relay 0.8. This appears to be the preferred design now that the typed async SDK is available: it preserves the asynchronous and cancellation behavior required by this plugin while moving generic native-host ownership and executor concerns back into Relay. |
Complete plugin configuration support matrixThe following is illustrative pseudo-TOML, intended to show the complete configuration surface in one place. It is not directly parseable as written:
# Relay host configuration. This outer version is Relay's plugin-config version.
version = 1
[[plugins.dynamic]]
manifest = "/opt/switchyard-relay-plugin/relay-plugin.toml"
[plugins.dynamic.config]
# This inner version is the Switchyard plugin configuration version.
version = 2
priority = <integer> # default: 0
max_retries = <integer from 0 through 10> # default: 3; each retry starts a fresh libsy run
# Each present key enables that caller protocol and names its trusted fallback.
# Omit a key to leave that inbound protocol unmanaged by this plugin.
[plugins.dynamic.config.default_targets]
openai_chat = "chat_fallback" || omit
openai_responses = "responses_fallback" || omit
anthropic_messages = "anthropic_fallback" || omit
# Repeat this target table under a unique semantic name for every serving or
# judge target referenced above or by the selected algorithm.
[plugins.dynamic.config.targets.<semantic_target_name>]
model = "<provider model id>"
protocol = "openai_chat" || "openai_responses" || "anthropic_messages"
base_url = "http://<provider>" || "https://<provider>"
endpoint = "" || "/<canonical protocol endpoint>" || omit
weight = 0 || <positive number> || omit # default: 1; used by random only; 0 is fallback-only
drop_caller_extra_body = true || false || omit # default: false
extra_body = { <non-secret provider defaults> } || omit
# header_env is the only supported custom-header source. Values name
# environment variables; they are not literal header values.
[plugins.dynamic.config.targets.<semantic_target_name>.header_env]
authorization = "PROVIDER_AUTHORIZATION" || omit
<another_header_name> = "<ENVIRONMENT_VARIABLE_NAME>" || omitAlgorithm Option A: seeded weighted random[plugins.dynamic.config.algorithm]
kind = "random"
seed = <nonnegative integer> || omit
# Selection weights come from each target's `weight` field. At least one target
# must have a positive weight. A zero-weight target remains available as a
# trusted fallback but is excluded from random selection.Algorithm Option B: capability classifier[plugins.dynamic.config.algorithm]
kind = "llm_classifier"
mode = "capability" || omit # omitted mode defaults to capability
classifier_target = "judge"
weak_target = "weak"
strong_target = "strong"
base_threshold = <number from 0 through 1>
threshold_step = <nonnegative number> || omit # default: 0; base + 2*step must be <= 1
recent_turn_window = <nonnegative integer> || omit
max_output_tokens = <positive integer> || omit # default: 4096
prompt = "<custom judge prompt>" || omit
session_affinity = true || false || omit # default: false
message_hash_fallback = true || false || omit # default: falseAlgorithm Option C: escalation classifier[plugins.dynamic.config.algorithm]
kind = "llm_classifier"
mode = "escalation"
classifier_target = "judge"
weak_target = "weak"
strong_target = "strong"
prompt = "<custom escalation-judge prompt>" || omit
max_output_tokens = <positive integer> || omit # default: 4096
[plugins.dynamic.config.algorithm.escalation]
confirmations = <integer >= 1> || omit # default: 2
recent_turn_window = <integer >= 1> || omit # default: 28
window_message_chars = <integer >= 50> || omit # default: 500
# confirmations > 1 requires a stable Switchyard session ID for the streak and
# strong-target latch to persist across turns.Algorithm Option D: stage router[plugins.dynamic.config.algorithm]
kind = "stage_router"
capable_target = "strong"
efficient_target = "weak"
picker = "capable_first" || "efficient_first"
confidence_threshold = <number from 0 through 1>
recent_turn_window = <nonnegative integer> || omit
capable_system_prompt = "<capable-tier prompt>" || omit
efficient_system_prompt = "<efficient-tier prompt>" || omit
# The complete handoff_notes table is optional. If present, escalation_note is required.
[plugins.dynamic.config.algorithm.handoff_notes]
escalation_note = "<note sent to the capable tier>"
deescalation_note = "<note sent to the efficient tier>" || omit
only_on_wrong_signal_escalation = true || false || omit # default: true
# The complete classifier table is optional and is used only when tool signals
# are ambiguous. Without it, the configured picker supplies the default tier.
[plugins.dynamic.config.algorithm.classifier]
target = "judge"
base_threshold = <number from 0 through 1>
threshold_step = <nonnegative number> || omit # default: 0; base + 2*step must be <= 1
recent_turn_window = <nonnegative integer> || omit
prompt = "<custom fallback-classifier prompt>" || omit
max_output_tokens = <positive integer> || omit # default: 4096Protocol and execution behavior represented by this configuration:
Current known limitation: |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
I reproduced the current Rust failure at PR head The break is the #373
A local six-file patch (
I have not pushed to the contributor branch. @bbednarski9, I can provide the exact patch if useful, or a maintainer can apply this narrow migration while updating the branch from main. Two non-code release gates remain for downstream canonical consumers:
|
6 participants
What
Adds the external
nvidia.switchyardNeMo Relay native plugin as an isolated,nested Cargo workspace under
crates/switchyard-nemo-relay-plugin. The pluginexecutes libsy's
Algorithm::run_streamlifecycle through the recommendedswitchyard-llm-client::runhost driver, which serves offloaded model callsusing plugin-owned HTTP clients. This requires neither a Switchyard service nor
changes to core Switchyard crates.
The integration supports:
session latching; and
classifier fallback.
Buffered and streaming callers are supported for OpenAI Chat, OpenAI Responses,
and Anthropic Messages. Provider headers are sourced through
header_env(per Relay convention), soliteral secrets are not stored in plugin configuration. Routing-only LLM calls
emit ATOF marks with normalized usage so judge and discarded-candidate compute
can be accounted for without double-counting the final serving call.
Router capability matrix
randomstage_routerAPI-level compatibility
name,schema, andstrictbelowtext.format.json_schemainstead of placing them directly belowtext.formatSame-protocol streaming preserves parsed provider JSON events when the router
does not aggregate or replace them. Raw SSE framing is not part of the contract.
Cross-protocol streams use normalized chunks.
Why
Switchyard algorithms need to own the complete routing lifecycle so policies can
inspect intermediate responses and make multiple provider calls. Keeping this
inside the plugin preserves that lifecycle while integrating with NeMo Relay's
native plugin API.
The implementation is intentionally isolated from the core library. The PR is
based directly on current
mainand contains no tracked modifications toSwitchyard core/libsy files; the only non-plugin changes are documentation
entries in
README.md,CHANGELOG.md, anddocs/index.md.Related: #192, #220, #271, #274, NVIDIA/NeMo-Relay#594
How tested
uv run ruff check .clean (N/A: Rust-only plugin)uv run mypy switchyardclean (N/A: Rust-only plugin)uv run pytest tests/green (N/A: Rust-only plugin)cargo test --workspace --all-targetscargo clippy --workspace --all-targets -- -D warnings-D warningscargo fmt --all -- --checkgit diff --checkThe three hosted failures are all OpenAI Responses structured-output judge
variants: capability classification, escalation judging, and the staged
classifier fallback. Each reached the expected judge-failure/fall-open path
after InferenceHub returned
Missing required parameter: 'text.format.name'.Random routing, every caller protocol, every ordinary serving-target protocol,
OpenAI Chat judges, Bedrock Sonnet staged judging with sanitized tool history,
stream reconstruction, escalation streak/latching, missing-session isolation,
stage signal selection, prompts, and handoff notes passed.
Checklist
snake_caseof the primary class. (N/A: no Python classes added.)switchyard/__init__.py.__all__if intended for downstream use. (N/A: Rust plugin surface.)--helpupdated if customer-facing surface changed.Signed-off-by: Your Name <email>) per the DCO.Notes for reviewers
manifests and lockfiles are unchanged.
surface, so the plugin currently uses a small raw-FFI ownership adapter.
NeMo Relay plans to provide the equivalent safe typed facade in 0.8.0; once
that is available, the raw-FFI compatibility adapter should be removable.
Switchyard intercept. Routing-only model calls are represented by
switchyard.routing.llm_callATOF marks; the successful serving call remainsrepresented by Relay's outer LLM lifecycle event.
provider-neutral text, convert tool-role messages to a neutral role, and send
no native tool definitions or tool-choice configuration. This avoids Bedrock's
tool calling without toolsrejection while preserving the routing evidence.response is buffered and judged. If weak wins, the caller receives a
reconstructed stream; preserved provider-event envelopes are not guaranteed
on that buffered path.
confirmations > 1requires a stable session ID. Requests withoutone remain isolated and cannot build a latch streak.
header_envexclusively. Target URLs containingcredentials, query parameters, or fragments are rejected during loading.
Adjacent hardening
This PR remains self-contained. Broader library work is deliberately kept out
of its review scope:
switchyard-llm-clientconsumers.LossyConversionPolicyfor buffered response encoding.switchyard-translationencoder correction plus core and process-levelregression coverage. Until then, configure judges with
protocol = "openai_chat".