Repository navigation
feat: record the routing condition an eval runs under - #2600
linj-glitch wants to merge 0 commit into
Conversation
Test resultsPre-commit clean. Since the PR opened, four unit tests were added for the record's failure paths: an unwritable Live validation (real
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| deployment_sha256: Optional[str] = None | ||
| deployment_toml: Optional[str] = None | ||
| if self.config.launches_proxy: | ||
| deployment_bytes = Path(str(self.config.deployment)).read_bytes() |
There was a problem hiding this comment.
RISK — deployment read is outside the best-effort guard; can crash startup and leak the proxy.
WHAT BREAKS: read_bytes() (and the sha256/re.sub that follow) run before the try: at line 328. The docstring promises "failing to write it must not take down serving," but an OSError from read_bytes() — transient NFS/permission hiccup, or the deployment file racing away after start_proxy() opened it — propagates out of write_condition_manifest(), which is called at line 228 before the try/finally that owns stop_proxy() (lines 232-238). So the exception both fails startup and leaks a proxy left serving — exactly the waste the comment at lines 229-231 warns against.
BLAST RADIUS: narrow probability (the native Server already read the TOML at start_proxy), but the failure mode is the one this method exists to avoid, and the proxy leak compounds it.
FIX: move the read_bytes/hash/redact block inside a try/except OSError that logs a warning and leaves deployment_sha256/deployment_toml as None, or widen the existing try: to wrap from line 305.
| deployment_sha256 = hashlib.sha256(deployment_bytes).hexdigest() | ||
| # The native schema references credentials by environment variable name, but an | ||
| # inline literal would be a secret -- drop the value, keep the line visible. | ||
| deployment_toml = re.sub( |
There was a problem hiding this comment.
RISK — redaction is a single-field denylist; other inline secrets leak into an artifact the docs call "safe to keep."
WHAT BREAKS: the regex redacts only api_key = .... The docs (switchyard.mdx: "credential values redacted"; PR body: "safe to keep next to results") tell users the archived TOML is safe to commit/share. But any other inline secret in the deployment survives verbatim — e.g. an Authorization = "Bearer sk-..." in a client headers/default_headers table, or any credential field the Switchyard schema accepts inline besides api_key. Those land in switchyard-condition.json next to eval outputs users are explicitly encouraged to archive.
BLAST RADIUS: credential exposure in a file promised to be sanitized — worse than no redaction, because it manufactures false confidence.
FIX: prefer an allowlist — parse the TOML, keep only the non-secret keys you intend to publish, drop the rest. If api_key genuinely is the only inline secret the schema permits, scope the guarantee: say so in the docstring and docs and cite the schema, rather than implying blanket redaction. As a stopgap, add Authorization/bearer/token patterns to the regex.
| deployment_sha256: Optional[str] = None | ||
| deployment_toml: Optional[str] = None | ||
| if self.config.launches_proxy: | ||
| deployment_bytes = Path(str(self.config.deployment)).read_bytes() |
There was a problem hiding this comment.
RISK — deployment read is outside the best-effort guard; can crash startup and leak the proxy.
WHAT BREAKS: read_bytes() (and the sha256/re.sub that follow) run before the try: at line 328. The docstring promises "failing to write it must not take down serving," but an OSError from read_bytes() — transient NFS/permission hiccup, or the deployment file racing away after start_proxy() opened it — propagates out of write_condition_manifest(), which is called at line 228 before the try/finally that owns stop_proxy() (line 232-238). So the exception both fails startup and leaks a proxy left serving — exactly the waste the comment at lines 229-231 warns against.
BLAST RADIUS: narrow probability (the native Server already read the TOML at start_proxy), but the failure mode is the one the method exists to avoid, and the proxy leak compounds it.
FIX: move the read_bytes/hash/redact block inside a try/except OSError that logs a warning and leaves deployment_sha256/deployment_toml as None, or widen the existing try: to wrap from line 305.
| deployment_sha256 = hashlib.sha256(deployment_bytes).hexdigest() | ||
| # The native schema references credentials by environment variable name, but an | ||
| # inline literal would be a secret -- drop the value, keep the line visible. | ||
| deployment_toml = re.sub( |
There was a problem hiding this comment.
RISK — redaction is a single-field denylist; other inline secrets leak into an artifact the docs call "safe to keep."
WHAT BREAKS: the regex redacts only api_key = .... The docs (switchyard.mdx: "credential values redacted", PR body: "safe to keep next to results") tell users the archived TOML is safe to commit/share. But any other inline secret in the deployment survives verbatim — e.g. a default_headers/client headers entry carrying Authorization = "Bearer sk-...", or any credential field the Switchyard schema accepts inline besides api_key. Those land in switchyard-condition.json alongside eval outputs users are actively encouraged to archive.
BLAST RADIUS: credential exposure in a file promised to be sanitized — worse than no redaction because it manufactures false confidence.
FIX: either (a) redact by an allowlist/parse — load the TOML, keep only the non-secret keys you intend to publish, drop everything else; or (b) if api_key is genuinely the only inline secret the schema permits, say so explicitly in the docstring and docs and cite the schema, so the guarantee is scoped rather than implied. Also consider Authorization/bearer/token header patterns in the regex as a stopgap.
|
🌿 Preview your docs: https://nvidia-preview-feat-switchyard-condition-provenance.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
github-actions
Bot
added
the
sla:review-overdue
e9f7192 to
748acd9
Compare
c4923e8 to
748acd9
Compare
|
Folded into #2141 to keep a single PR — the condition-provenance commits are now on |
…ion provenance (#2141) Part of #2088. This PR now also absorbs #2600 (routing-condition provenance) so the whole feature reviews in one place. Adds `responses_api_models/switchyard_model/`, a model server that routes Gym model calls through a Switchyard proxy, so any benchmark Gym supports can run against a router without harness changes — unlike the per-harness integrations in #2026/#2080, which are right for RL trace capture but not for evaluating the router itself. Gym either hosts the router in-process from a native Switchyard TOML deployment (`deployment=routes.toml`; a bad deployment fails at startup, nothing outlives the process, and multi-worker configs are rejected since each worker would host its own proxy) or attaches to an externally run proxy via `switchyard_base_url`, which does support multiple workers. A caller-supplied `model` is always overridden with the configured route so an agent can't bypass the router, and Gym forwards its rollout-attempt id as the Switchyard session id (`x-switchyard-session-id`) so routing decisions and costs join back to the rollout that produced them. That id only arrives when model-call capture or token capture is enabled, so the server validates the requirement at startup: a warning under the default config, a refusal to start when forwarding was requested explicitly but no capture is on. From #2600: set `condition_dir` and the server records the routing condition the run served under — `switchyard-condition.json` at startup (route, mode, proxy URL, deployment SHA-256 plus an archive with inline `api_key` and all `extra_headers` values redacted, and the proxy's identity: the local wheel version when hosting, a caller-supplied `proxy_provenance` mapping when attaching) and `switchyard-stats.json` at shutdown (per-target requests, errors, tokens, and latency plus classifier-side usage from `/v1/stats`, fetched through Gym's shared async client as the configured caller and hard-capped inside the shutdown window). The snapshot labels its scope, since a hosted proxy's counters cover exactly the run while an attached proxy's aggregate everything it has served. Both writes are best-effort and cannot take down serving. This makes two routed runs comparable and a hosted run reproducible from the archived deployment; the docs page gains a "Compare routing conditions" workflow that aligns rollout rows by task and rollout index, reports rollouts missing a counterpart, and includes the classifier tokens the selected model's usage omits. The only dependency is `nemo-switchyard==0.2.0`, pinned exactly in this server's own pyproject.toml (Gym core never imports Switchyard); 0.2.0 is the floor because it is the first release whose Responses translation passes `NeMoGymResponse` validation (NVIDIA-NeMo/Switchyard#144), and the ceiling because upstream has already removed the Python serve stack this PR originally drove. 47 tests pass with 100% coverage on app.py, including an integration suite that drives the real 0.2.0 wheel with no mocks (chat/responses round trips, route override, stats, provenance files written across a real shutdown), and a live routed rollout ran against this revision with reward 1.0 (results in [this comment](#2141 (comment))). Two reviewer notes. `ModelCallRecord.model_ref` names the Gym model server, not the routed target, so per-call attribution must be read from captured payloads — promoting it to a first-class record field is the follow-up flagged for #2088 and needs a core-schema design pass. And `harbor_agent` overwrites `response["model"]` with the policy model name, so Harbor-run benchmarks report the route name for every call; flagging in case that overwrite is unintentional. --------- Signed-off-by: Lin Jia <linj@nvidia.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Part of #2088 (routing-aware evals). Against that epic's acceptance criteria, this PR delivers:
condition_dirmanifest + stats recordThe remaining criterion (docs: when/why Gym + Switchyard) is largely covered by #2141's docs page; the deeper follow-on — per-call routed-target attribution as a first-class
ModelCallRecordfield — is flagged in #2141's reviewer notes and needs a core-schema design pass.Stacked on #2141 — retarget to
mainafter it merges; only the last commits are new.What
Makes routed runs identifiable, comparable, and reproducible from their outputs. Set
condition_dir(one directory per run) onswitchyard_modeland the server records the routing condition the run served under:switchyard-condition.json(written at startup): the route, mode (hosted/attached), proxy root URL,nemo-switchyardversion, and the deployment's SHA-256 plus archived contents — with credential values redacted, so the archive is safe to keep next to results.switchyard-stats.json(written at shutdown): the proxy's/v1/stats— per-target requests, tokens, retries, latency — captured before the in-process server goes away. For a hosted proxy these counters would otherwise die with the process. Works in attach mode too.Both writes are best-effort: a provenance failure logs a warning rather than taking down serving.
Why
#2088's core acceptance criteria are "routing condition identifiable in eval outputs across runs" and "documented workflow comparing ≥2 routing conditions." Before this change, two runs of the same benchmark under different deployments produced byte-identical-shaped outputs with nothing saying which condition was which, and the proxy-side cost signals were unrecoverable after shutdown.
Two runs are fairly comparable when their manifests differ only in
routeor deployment hash; a run is reproducible from the archived deployment alone. Everything stays insideresponses_api_models/switchyard_model/— Gym core is untouched, preserving #2141's dependency boundary. (Promoting per-row condition/attribution fields into the core rollout schema is the deeper follow-up flagged in #2141's reviewer notes.)Docs
The Switchyard page gains a Compare routing conditions section: run the same benchmark once per condition (the page's example deployment already defines a
strong-onlybaseline and apolicy-modelrouter), each with its own output file andcondition_dir, then compare mean reward and token totals straight from the rollout rows — with the manifest and stats snapshot explaining what routing did. A tip covers stateful (session-affinity) strategies via a shared attach-mode proxy. Against #2088's checklist, this closes the comparison-workflow and condition-identifiability criteria; the end-to-end example criterion is the walkthrough plus the validated deployment on the docs page.Testing
31 passed(the 26 from #2141 plus 5 new), pre-commit clean.condition_dirunset writes nothing, unreachable stats endpoint doesn't fail shutdown.🤖 Generated with Claude Code