[NV] [PR1] feat(synthetic-al): generic synthetic-acceptance injection scaffold

[qiching]

No description provided.

[claude]

Claude finished @qiching's task in 4m 6s —— View job

---

Review of PR #2088 — synthetic-acceptance injection scaffold

• Gather context (PR diff vs origin/main, read all 3 changed files)
• Review runners/inject_synthetic_acceptance.py (driver logic, env handling, YAML lookup)
• Review runners/synthetic_injectors/__init__.py (registry scaffold)
• Review runners/launch_gb200-nv.sh changes (injection hook, dsv4 fp4 path, shared-FS routing)
• Post findings

Summary

Found 1 issue (posted inline): the injection hook at runners/launch_gb200-nv.sh:476 doesn't propagate failures — the script has no set -e, and every error path in the injector is sys.exit(1), so with SYNTHETIC_ACCEPTANCE=true a failed injection (which is every opted-in run in this PR, since the registry is empty) silently degrades to a vanilla run whose results are then mislabeled as synthetic-acceptance. Suggested fix is appending || exit 1, matching the idiom already used elsewhere in this script.

Everything else checks out: the no-op default path is genuinely behavior-preserving, the synthetic_injectors import resolves correctly (script dir is on sys.path), the missing benchmarks/speedbench-reference-al.yaml is handled with an explicit error and is unreachable until the follow-up backend PR, and the dsv4 fp4 MODEL_PATH / uses_watchtower_shared_fs changes are deliberate and well-documented.

[claude]

🟡 WARNING: Injector failures are silently swallowed — the run proceeds without synthetic acceptance.

Why it matters: This script has no set -e (only set -x). Every failure path in inject_synthetic_acceptance.py is a sys.exit(1) — no registered backend, missing benchmarks/speedbench-reference-al.yaml, unknown model key, unmatched num_spec_tokens, or an ImportError if PyYAML is absent from the venv. With SYNTHETIC_ACCEPTANCE=true, any of those prints an error into the set -x log noise and the launch continues, submitting the benchmark without injection — producing results labeled as synthetic-acceptance that actually ran vanilla MTP. In this PR the registry is empty, so every opted-in run would hit exactly this path.

Fix: fail the launch on injector error, matching the || exit 1 idiom already used elsewhere in this script:

Suggested change

	python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK"
	python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK" || exit 1

[claude]

🟡 The python3 inject_synthetic_acceptance.py invocation at line 476 lacks a || exit 1 guard, and the launcher only sets set -x (no set -e), so any sys.exit("ERROR: …") from the injector prints to stderr but bash proceeds to srtctl apply against the unmodified recipe — the sweep succeeds and produces well-formatted results that a user believing SYNTHETIC_ACCEPTANCE=true took effect would misread as synthetic-acceptance data. Fix is a one-token || exit 1 matching the file's own established pattern (lines 448 mkdir, 455 rsync, and every git-clone block).

Extended reasoning...

What the bug is. The launcher at runners/launch_gb200-nv.sh:476 invokes:

python3 "$GITHUB_WORKSPACE/runners/inject_synthetic_acceptance.py" "$CONFIG_PATH" "$FRAMEWORK"

with no exit-code guard. The file's shebang line runs only set -x (line 5) — grep -n '^set ' confirms no set -e anywhere — so a non-zero exit from the python3 call does not abort the script. Bash continues past it into unset VIRTUAL_ENV and then srtctl apply, submitting the unmodified recipe.

Why the failure paths are reachable on this PR as merged. inject_synthetic_acceptance.py has multiple sys.exit("ERROR: …") paths (missing backend at lines 127–131; missing reference YAML at 89–93; missing MODEL_PREFIX/THINKING_MODE/num_spec_tokens keys at 73/103/113; unhandled ValueError from float() on a non-numeric SYNTHETIC_ACCEPTANCE_LENGTH). On this scaffold PR two of them are guaranteed to fire on any opt-in: runners/synthetic_injectors/__init__.py registers no backends (_INJECTORS = {}, intentional per its docstring), and benchmarks/speedbench-reference-al.yaml does not exist in the tree.

Concrete step-by-step proof.

1. A user reads the new module's docstring and exports SYNTHETIC_ACCEPTANCE=true before launching a dsv4 dynamo-vllm sweep.
2. The launcher reaches line 476 and runs python3 …/inject_synthetic_acceptance.py $CONFIG_PATH dynamo-vllm.
3. Inside inject(), get_injector('dynamo-vllm') returns None (empty registry).
4. The script calls sys.exit("ERROR: SYNTHETIC_ACCEPTANCE=true but no synthetic-acceptance injector is registered for FRAMEWORK='dynamo-vllm'") → process exits with code 1.
5. Bash does not stop (no set -e, no || exit 1). The ERROR line prints to stderr, then execution continues at line 478: unset VIRTUAL_ENV.
6. srtctl apply -f "$CONFIG_PATH" … submits the recipe as it was after the sed name override — i.e. the plain speculative-decoding recipe, no synthetic-acceptance parameters injected.
7. The sweep completes and writes results_concurrency_*.json files with the standard schema. Downstream tooling and dashboards treat these numbers as synthetic-acceptance measurements.

Why existing safeguards don't catch it. No set -e; no explicit rc=$? check after the python3 call; no downstream assertion that the recipe was modified when SYNTHETIC_ACCEPTANCE=true. The ERROR text is present in stderr but interleaved with set -x trace output; an attentive operator watching a live run would notice, but sweep artifacts (JSON results, pass/fail status) look normal.

Impact. On this PR alone the impact is bounded: the docstring explicitly notes 'merging it changes no behavior' with the flag off, and no existing sweep sets SYNTHETIC_ACCEPTANCE=true. The bug becomes concrete data-integrity risk once the follow-up framework-support PR registers backends and the auto-lookup YAML lands — any remaining failure path (missing MODEL_PREFIX key, non-numeric LENGTH, backend registered for one framework but not another) would still silently mislabel results as synthetic-acceptance data.

How to fix. Add || exit 1 (or explicit rc handling) to line 476 — a one-token change that matches the file's own established pattern for critical operations: line 448 mkdir -p … || exit 1, line 455 rsync … || exit 1, and every git clone/git checkout/cp -rT block in the srt-slurm-setup section. The design contract of an opt-in flag is 'either apply the change or fail loudly'; silently ignoring failure is worse than never having the seam.