Skip to content

ar_validate: fail loudly when every sample fails #2288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yeyu-nvidia wants to merge 3 commits into
base: main
Choose a base branch
from
+18 −1
Open

ar_validate: fail loudly when every sample fails #2288

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad18184
ar_validate: fail loudly when every sample fails
yeyu-nvidia Aug 25, 2026
db802b1
address review: report the attempted sample count, not the requested one
yeyu-nvidia Aug 31, 2026
638ad6a
address review: distinguish zero samples from all-samples-failed
yeyu-nvidia Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion examples/speculative_decoding/scripts/ar_validate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,24 @@ def main():
accelerator.device,
)

if results and accelerator.is_main_process:
# validate_ar() clamps to len(ds), so report what was actually attempted rather than the
# requested --num_samples, which can be larger than the dataset. A non-positive count means
# nothing ran at all -- distinct from "everything ran and failed", so say so separately.
attempted = min(args.num_samples, len(ds))
if attempted <= 0:
raise ValueError(
f"No samples to validate: --num_samples={args.num_samples} with a dataset of "
f"{len(ds)} prompts. Pass a positive --num_samples."
)
Comment on lines +126 to +134

@claude claude Bot Aug 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] The non-positive-count guard runs after validate_ar(), so --num_samples 0 still pays for the full model load and accelerator.prepare() across all GPUs before erroring out. It also duplicates the min(num_samples, len(ds)) clamp that already lives at line 63, so the two can drift apart if either side changes.

Both go away if the block moves up to just after ds is loaded and before the validate_ar() call — len(ds) is available there, and it becomes a genuine "reject before processing" check rather than a post-hoc one:

    ds = load_dataset("HuggingFaceH4/mt_bench_prompts")["train"]

    # validate_ar() clamps to len(ds), so track what will actually be attempted rather than the
    # requested --num_samples, which can be larger than the dataset. A non-positive count means
    # nothing will run at all -- distinct from "everything ran and failed", so say so separately.
    attempted = min(args.num_samples, len(ds))
    if attempted <= 0:
        raise ValueError(
            f"No samples to validate: --num_samples={args.num_samples} with a dataset of "
            f"{len(ds)} prompts. Pass a positive --num_samples."
        )

    results = validate_ar(
        model,
        tokenizer,
        ds,
        args.steps,
        args.osl,
        args.num_samples,
        accelerator.device,
    )

    if not results:
        raise RuntimeError(
            f"AR validation produced no results: all {attempted} samples failed. "
            "See the per-sample WARNING lines above for the underlying error. "
            "Exiting non-zero so this is not mistaken for a successful validation."
        )

Non-blocking — the current ordering is functionally correct (the empty loop yields no results, and attempted <= 0 is checked before not results, so the two failure modes still get distinct messages). This only saves a wasted load on a typo'd flag and keeps the clamp in one place.


if not results:

@claude claude Bot Aug 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] The guard is all-or-nothing, so a near-total failure still exits 0 with a meaningless AR.

The reasoning in the PR description — "a run where 100% of samples failed was indistinguishable from a successful one" — applies just as well at 98.75%. If 79 of 80 samples die and one survives, avg_ar is computed from that single sample, printed as a normal result, and the script exits 0; a wrapper that greps for an AR number stamps PASS on noise. The motivating case (device_map="auto" device mismatch) happens to fail every sample, but a per-prompt failure mode — OOM on the longest MT-Bench prompts, a tokenizer edge case — fails a subset and lands squarely in this gap.

print(f" Samples: {len(results)}") does leave the evidence in the log, so this is a human-readable signal, not an automated one. Worth considering a failure-rate bound rather than an emptiness check, e.g. have validate_ar return the attempted/failed counts and fail when the failure fraction exceeds a threshold (a --max_failure_rate with a lenient default would keep flaky-but-usable runs green while still catching a collapse):

results, attempted, failures = validate_ar(...)

if failures / attempted > args.max_failure_rate:
    raise RuntimeError(
        f"AR validation failed for {failures}/{attempted} samples, above the "
        f"--max_failure_rate of {args.max_failure_rate}. See the per-sample WARNING "
        "lines above for the underlying error."
    )

Reasonable to defer as out of scope for a targeted bug fix — the current change is a strict improvement either way.

@yeyu-nvidia yeyu-nvidia Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the gap is real — 79/80 failing still prints an AR from one sample and exits 0, and the argument I make in the description does apply there. Postponing rather than folding it in here: a failure-rate bound changes behavior for runs that currently pass, so it deserves its own PR with a default chosen deliberately (and probably a --max_failure_rate flag rather than a hardcoded threshold). This PR stays scoped to the 100%-failure case, which is unambiguous.

Separately, thanks for the eagle_utils.py catch — I verified it: validate_ar() returns (category, ar) tuples and eagle_utils.py:415/417 does sum(ars) / len(ars), which raises TypeError straight into the surrounding bare except Exception. So in-training AR validation prints "AR validation not available." and never logs to W&B. Real bug, same silent-failure family, but a different file and call path than this PR touches — filing separately.

raise RuntimeError(
f"AR validation produced no results: all {attempted} samples failed. "
"See the per-sample WARNING lines above for the underlying error. "
"Exiting non-zero so this is not mistaken for a successful validation."
)

if accelerator.is_main_process:
all_ars = [ar for _, ar in results]
avg_ar = sum(all_ars) / len(all_ars)
print(f"\n==== AR Validation Results (osl={args.osl}, steps={args.steps}) ====")
Expand Down