[NV] [PR2] feat(synthetic-al): vLLM backend for synthetic-acceptance injection

[qiching]

No description provided.

[claude]

Claude finished @qiching's task in 2m 55s —— View job

---

Review

• Read the PR diff and changed files
• Verify injector behavior and launcher error handling
• Post review feedback

Found 1 blocking issue and 1 warning, both about the same failure mode: a run that opts in with SYNTHETIC_ACCEPTANCE=true can silently fall through to real speculative decoding and record those numbers as synthetic-acceptance results.

• 🔴 runners/launch_gb200-nv.sh:497 — the injector call has no || exit 1 and the script has no set -e, so all of the Python script's fail-loud sys.exit paths are ignored and the unmodified recipe is submitted anyway.
• 🟡 runners/inject_synthetic_acceptance.py:146-148 — the "no speculative-config entries matched" case warns and exits 0; for an explicitly opted-in run this should be a hard error (the regex only matches single-quoted JSON, so an upstream recipe formatting change would silently disable injection).

One heads-up (not blocking, assuming it lands in the stacked follow-up): benchmarks/speedbench-reference-al.yaml does not exist in the repo yet — the speedbench scripts only write it to $RESULTS_DIR — so the AL auto-lookup path can currently never succeed and enabled runs must set SYNTHETIC_ACCEPTANCE_LENGTH explicitly. The registry/backend split itself, the module-level import path, and the JSON rewrite logic all check out.

[claude]

🔴 BLOCKING: Injector failure is silently ignored — the script's exit code is never checked.

Why it matters: This script has no set -e (only set -x), so every sys.exit("ERROR: ...") path in inject_synthetic_acceptance.py (missing reference YAML, unknown MODEL_PREFIX/num_spec_tokens, no injector registered for the framework) prints an error and then the launcher continues and submits the unmodified recipe. A sweep that explicitly opted in with SYNTHETIC_ACCEPTANCE=true would run real speculative decoding and record those numbers as synthetic-acceptance results — a silent data-corruption failure mode. All the fail-loud logic in the Python is defeated by this one line. Every neighboring critical command in this file uses || exit 1 (e.g. lines 458, 469, 476).

Fix:

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]

🟡 WARNING: When injection is explicitly enabled but nothing matches, this warns and exits 0 — the benchmark proceeds with real speculative decoding while the run is labeled synthetic.

Why it matters: The vLLM backend's regex only matches single-quoted JSON (speculative-config: '<json>'). If an upstream srt-slurm recipe switches to double quotes, block style, or a speculative_config key spelling, count is 0 and the sweep silently records real-MTP numbers as synthetic-acceptance results. A run that set SYNTHETIC_ACCEPTANCE=true has no legitimate reason to have zero speculative-config entries, so this should fail loudly like the other error paths.

Fix:

Suggested change

	if count == 0:
	_log("WARNING: No speculative-config entries found to modify; leaving recipe unchanged")
	return 0
	if count == 0:
	sys.exit("ERROR: SYNTHETIC_ACCEPTANCE=true but no speculative-config entries were found in the recipe")

[claude]

🔴 The synthetic-acceptance injection at runners/launch_gb200-nv.sh:497 is invoked without || exit 1, and the launcher does not use set -e, so any non-zero exit from inject_synthetic_acceptance.py is silently discarded. If a user opts into SYNTHETIC_ACCEPTANCE=true but misconfigures the run (e.g., unregistered framework, MODEL_PREFIX missing from speedbench-reference-al.yaml, missing num_spec_tokens / THINKING_MODE), the driver's sys.exit("ERROR: ...") guardrails fire but srtctl apply still proceeds with the ORIGINAL, uninjected recipe — reporting benchmark numbers as if synthetic AR was applied when it was not. Fix by appending || exit 1 to the python3 call, matching the convention already used by the nearby mkdir -p ... || exit 1 and rsync ... || exit 1 calls in this file.

Extended reasoning...

The bug

At runners/launch_gb200-nv.sh:497:

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

The launcher has set -x at the top (line 5) but no set -e — verified by grep. The injection call has no || exit 1 guard and its exit status is not checked before the script continues on to unset VIRTUAL_ENV and eventually srtctl apply.

Why the guardrails don't work

inject_synthetic_acceptance.py uses sys.exit("ERROR: ...") for five distinct opt-in misconfiguration paths, all of which are intended as hard failures when the user opts in to synthetic AR but misconfigures it:

1. get_injector(framework) returns None — no backend registered (only dynamo-vllm is registered today, so dynamo-sglang / dynamo-trt will hit this).
2. Reference YAML speedbench-reference-al.yaml missing.
3. MODEL_PREFIX-derived key not found in the reference YAML.
4. num_spec_tokens=<n> not found for that model.
5. THINKING_MODE not found in a thinking-matrix block.

Python's sys.exit(<str>) prints the message to stderr and exits with status 1. Bash silently discards that non-zero exit, and the launcher continues past the injection call.

The impact

Concrete failure mode: a user sets SYNTHETIC_ACCEPTANCE=true and MODEL_PREFIX=minimaxm2.5 (no MODEL_PREFIX_TO_YAML_KEY mapping, likely no top-level key in speedbench-reference-al.yaml). The driver logs ERROR: model key "minimaxm2.5" not found ... and exits 1, but the launcher proceeds to srtctl apply with the unmodified recipe. The sweep then runs against the real draft model and reports numbers under the assumption of synthetic AR — a silent methodological mismatch that poisons the AL curve.

Step-by-step proof

1. User sets env: SYNTHETIC_ACCEPTANCE=true MODEL_PREFIX=minimaxm2.5 FRAMEWORK=dynamo-vllm.
2. Launcher hits line 497: python3 .../inject_synthetic_acceptance.py "$CONFIG_PATH" dynamo-vllm.
3. Inside inject(): SYNTHETIC_ACCEPTANCE == "true" → proceed. get_injector("dynamo-vllm") returns the registered backend → proceed.
4. _resolve_al reads speedbench-reference-al.yaml, computes key = _yaml_key("minimaxm2.5") = "minimaxm2.5" (not in MODEL_PREFIX_TO_YAML_KEY), then data.get("minimaxm2.5") returns None.
5. sys.exit('ERROR: model key "minimaxm2.5" not found in ...') → process exits with status 1.
6. Bash: $? is now 1. No || exit 1, no set -e. Execution continues.
7. Line 501+: unset VIRTUAL_ENV; srtctl apply -f "$CONFIG_PATH" ... runs against the unmodified $CONFIG_PATH.
8. Benchmark completes and result JSONs are tagged gb200,minimaxm2.5,fp8,...,infmax-YYYYMMDD — with no indication that synthetic AR was requested but not applied.

Consistency with the file's own convention

The launcher already establishes the pattern that critical steps must guard their exit codes: line 469 (mkdir -p ... || exit 1), line 476 (rsync ... || exit 1), and the whole elif ... minimaxm3 clone/checkout block chains || exit 1 on every step. The synthetic-AR injection is at least as consequential (silent-bad-benchmark-numbers vs. a fast failure) — the omission reads as an oversight, not a deliberate choice.

The fix

Append || exit 1 to the python3 call:

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

One character trailing pipe. Matches the file's own convention. Preserves the driver's no-op-on-opt-out behavior (a successful exit 0 still falls through) and makes opt-in misconfiguration abort loudly, which is exactly what the driver's sys.exit guardrails were trying to establish.