fix: Salt failure is a hard error - #406
Conversation
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces early validation for dataset salting during the benchmark setup phase. It adds a validate_saltable method to ensure that all dataset samples are compatible with salting (i.e., they are dictionaries containing a string prompt and no input_tokens) before any subprocesses are spawned. The review feedback highlights a critical bug in validate_saltable where iterating over self.data directly will fail if it is a pandas.DataFrame (as it would iterate over column names instead of rows), and provides a code suggestion to handle this case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Review Council — PR #406 (3× Claude, depth: thorough)Three independent Claude reviewers (bugs/correctness, design/edge, testing/docs) plus a code-quality pass, deduped and severity-recalibrated. "R" marks issues flagged independently by multiple reviewers. All line numbers verified against HEAD ( Verdict: Solid, focused bug-fix. Fail-fast placement is correct — validation at 🟡 Should fix
🔵 Consider
🧹 Code quality
Convergence: all three reviewers independently landed on the salt-default- |
- Default warmup.salt to False so pre-tokenized (input_tokens) workloads no longer hard-fail on a plain --warmup; the hard error now fires only when salt is explicitly enabled. - Replace raw-string salt-violation reasons with a typed DatasetValidationError.Reason enum plus optional detail; _salt_violation becomes _can_salt returning the enum. UNSPECIFIED covers not-yet-mapped --dataset parse errors. - validate_saltable: assert on unloaded data (no silent skip); clarify the error index is into the loaded post-transform order; document the intentional whole-dataset (fail-on-any-invalid) strictness and the deliberate pre-spawn check. - Tests: unpatched integration coverage (offline/online raise, accuracy-only skip), agentic messages sample, typed-reason assertions; drop brittle index regex. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: arekay-nv <230885705+arekay-nv@users.noreply.github.com>
Warmup without --warmup-salt issues prompts verbatim, priming the server KV/prefix cache so the measured phase can be served warm and understate latency. Surface it as a warning at dataset-load time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nv-alicheng
left a comment
nv-alicheng
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Codex + Claude + Code-Quality | Depth: standard
Found 3 issues (1 cross-confirmed by two reviewers). Overall a clean, well-tested refactor — typed Reason enum, fail-fast placement, and the collapsed _apply_salt are sound.
| # | File | Line | Severity | Category | Reviewer(s) | Summary |
|---|---|---|---|---|---|---|
| 1 | dataset_manager/dataset.py |
279 | medium | regression/api-contract | Codex + Claude | Multimodal list-form prompts now hard-error at load; list prompts were previously salted (first text part), not silently skipped — tested behavior removed. |
| 2 | commands/benchmark/cli.py |
70 | low | code-quality | Code-Quality | Comment narrates future work ("aren't yet mapped") without a tracking issue. |
| 3 | dataset_manager/dataset.py |
471 | low | documentation | Claude | validate_saltable docstring omits messages (code also rejects it). |
#1 is the decision point: the "silent skip" justification doesn't hold for list-form prompts — confirm the multimodal-salt drop is intended (and give list prompts a distinct Reason), or restore the branch.
| return Reason.MESSAGES_SHADOWING | ||
| if "prompt" not in sample: | ||
| return Reason.PROMPT_MISSING | ||
| if not isinstance(sample["prompt"], str): |
There was a problem hiding this comment.
[Codex + Claude] medium (regression / api-contract): This str-only check makes multimodal list-form prompts hard-error at load with PROMPT_TYPE_MISMATCH. Both reviewers independently verified that list prompts were previously salted correctly by prefixing the first type == "text" part (covered by the now-deleted test_multimodal_list_prompt_first_text_part_is_salted / test_multimodal_image_first_text_at_index_1_is_salted) — they were not silently skipped. So the PR's "salt was silently skipped" premise doesn't hold for the list case: this removes tested, working behavior, and any existing warmup.salt: true VLM/multimodal config now aborts at setup.
Two options:
- If dropping multimodal salt is intended, call it out as a breaking change and give list prompts a distinct, actionable
Reason— the current message ("sample 'prompt' is not a str") is misleading, since list prompts can be salted. - If unintended, restore the first-text-part salting branch for list prompts.
nv-alicheng
left a comment
nv-alicheng
left a comment
There was a problem hiding this comment.
Approved, but comments need to be addressed.
| enabled: false # Enable warmup phase before performance run | ||
| n_requests: null # Warmup request count (None = full dataset once) | ||
| salt: true # Prepend a unique random hex salt to each warmup prompt | ||
| salt: false # Prepend a unique random hex salt to each warmup prompt. Requires text-'prompt' samples; enabling it on a pre-tokenized ('input_tokens') dataset is a hard error. |
There was a problem hiding this comment.
Changing the default behavior to false might fail a lot of the submission silently (simply because the submitter forgot to turn them on).
Can we warm up with random token if the salt cannot be done, or some other alternative approach (e.g. add random tokens)? Based on the endpoints v0.7 and inference 6.1 I am really worried a non submit-compatible default flag will invalidate many submissions
There was a problem hiding this comment.
By default, both warmup and warmup salting are disabled. Enabling warmup alone leaves salting disabled; the run logs a warning and proceeds with unsalted warmup requests. A hard setup error occurs only when both warmup and salt are enabled and the loaded dataset contains a sample the implementation cannot safely salt. The bug this PR addresses is that, previously, requesting salt could still result in an unsalted outgoing request—sometimes silently and sometimes with only a warning.
This change would ensure that warmup with salting is either observed correctly or flagged as an error.
There was a problem hiding this comment.
Yes, that part makes sense. But for submission, only a warmup with SALT will ever be accepted for single-turn/agentic LLM workload. Will "SALT by default enabled" work? Or can a ruleset force that to be true? Thinking about reducing human mistakes and amount of context knowledge here
This is given that all future LLM/VLM workload will use text prompt whenever possible
Address review-council + reviewer feedback on the salt-failure-hard-error change: - Rename _can_salt -> _check_unsaltable (truthy Reason == unsaltable) and fix the validate_saltable walrus so it raises when a sample IS unsaltable - Add DatasetParseError for --dataset string parse failures; remove the vestigial Reason.UNSPECIFIED - Rename the list-prompt reason to PROMPT_LIST_UNSUPPORTED (spec-accurate: an OpenAI list prompt is a batch / token-ID array, not necessarily multimodal) - Drop the duplicate cyclopts help= on WarmupConfig.salt; Field(description=) is the single source for --help and the YAML templates - Make DatasetValidationError copy/pickle-safe via a 3-tuple __reduce__ (restores __dict__/__notes__ state, matching CPython's default reducer) - Propagate typed CLI errors from audit phases (InputValidationError -> exit 2) instead of recasting them to ExecutionError (exit 4) - Raise a clear ValueError on conflicting scalar/nested --dataset keys instead of an uncaught TypeError that escaped to an exit-1 traceback - Document the newly reachable exit 2 in audit.py and compliance_audit_plan.md - Add regression tests for all of the above Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
main #437 refactored _load_datasets to return (dataloader, eval_configs). test_accuracy_only_skips_salt_validation is branch-only, so the merge kept its stale 3-tuple unpack — the sole site the merge missed. Adopt the 2-tuple form; perf_loader is still the first element, so the assertion is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
What does this PR do?
Makes salt failures a hard error instead of a silent skip. Previously, when
--warmup-saltwas on (the default) but a sample couldn't be salted,Dataset._apply_saltlogged a warning (or silently passed the sample through) and the warmup ran with no cache-busting — the exact condition salt exists to prevent, failing quietly.Now
salt=Truerequires every sample to be adictwith a text (str)promptand noinput_tokens; anything else raisesDatasetValidationErrorat dataset-load time, before any load is issued.Key changes:
Dataset.validate_saltable()— rejects non-dictsamples,input_tokens(pre-tokenized; adapters send these verbatim so a saltedpromptnever reaches the server), missingprompt, and non-strprompt. Error names the offending sample index + remediation._load_datasetswhenwarmup.enabled and warmup.salt, so an unsaltable dataset fails before worker/aggregator subprocesses spawn. Also called fromwith_salt()so the salting mechanism stays self-protecting._apply_saltcollapses to a single dict-merge ({**data, "prompt": f"[{salt}] {data['prompt']}"}) — the multimodal-list handling,input_tokenswarnings, and silent passthroughs are deleted; the contract is guaranteed upstream.promptthat is a list (image/video workloads) or a dataset withinput_tokens(e.g. gpt-oss-120b, DeepSeek-R1 via/v1/completions) will fail warmup instead of silently skipping salt. Fix: set--warmup-salt=false/warmup.salt: false.n_requestssubset would never issue. Stricter than the old per-sample skip; intended by the hard-error design.Type of change
Related issues
Testing
Checklist