-
Notifications
You must be signed in to change notification settings - Fork 27
fix: Salt failure is a hard error #406
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.
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
arekay-nv
wants to merge
12
commits into
main
Choose a base branch
from
arekay/salt_failure_hard_error
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9e6989d
Salt failure is a hard error
arekay-nv ba6bb2f
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv 6cdbe52
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv de68f37
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv a328bca
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv 74a0621
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv 44fc32d
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv ae02652
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv 6f6c165
fix: address review feedback on salt-failure hard error
arekay-nv e7e1008
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv a72e50d
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv 83a61d5
Merge branch 'main' into arekay/salt_failure_hard_error
arekay-nv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| from datasets import load_dataset, load_from_disk | ||
|
|
||
| from ..config.schema import APIType, ModelParams | ||
| from ..exceptions import DatasetValidationError | ||
| from .transforms import ( | ||
| ColumnFilter, | ||
| Transform, | ||
|
|
@@ -257,6 +258,24 @@ def load_from_huggingface( | |
| return ds[split].to_pandas() | ||
|
|
||
|
|
||
| def _can_salt(sample: Any) -> DatasetValidationError.Reason | None: | ||
| """Return the Reason a sample cannot be salted, or None if it can. | ||
|
|
||
| Salt requires a dict sample with a str 'prompt' and no 'input_tokens' (which | ||
| adapters send verbatim, so a salted 'prompt' would not reach the server). | ||
| """ | ||
| Reason = DatasetValidationError.Reason | ||
| if not isinstance(sample, dict): | ||
| return Reason.TYPE_MISMATCH | ||
| if "input_tokens" in sample: | ||
| return Reason.INPUT_TOKENS_SHADOWING | ||
| if "prompt" not in sample: | ||
| return Reason.PROMPT_MISSING | ||
| if not isinstance(sample["prompt"], str): | ||
| return Reason.PROMPT_TYPE_MISMATCH | ||
| return None | ||
|
|
||
|
|
||
| class Dataset: | ||
| """Class for loading and managing benchmark datasets. | ||
|
|
||
|
|
@@ -440,55 +459,62 @@ def load_sample(self, index: int) -> Any: | |
| data = self._apply_salt(data) | ||
| return data | ||
|
|
||
| def validate_saltable(self) -> None: | ||
| """Raise if any loaded sample cannot be salted. | ||
|
|
||
| salt requires a dict sample with a text ('str') 'prompt' and no | ||
| 'input_tokens' (adapters send those verbatim, so a salted 'prompt' would | ||
| never reach the server). A non-saltable sample is an error, not a silent | ||
| skip: skipping would leave the KV cache un-busted. Every sample is | ||
| checked — a single invalid item fails the run, because the seeded warmup | ||
| subset can draw any index and salt correctness is all-or-nothing. Called | ||
| before any load is issued — at benchmark setup and again from with_salt(). | ||
|
|
||
| Raises: | ||
| DatasetValidationError: naming the first offending sample. The index | ||
| is into the loaded, post-transform sample order, not the source | ||
| file line. | ||
| """ | ||
| assert self.data is not None, "Dataset not loaded. Call load() first." | ||
| for i, sample in enumerate(self.data): | ||
| reason = _can_salt(sample) | ||
| if reason is not None: | ||
| raise DatasetValidationError( | ||
| reason, | ||
| detail=( | ||
| f"sample {i} (index into the loaded, post-transform " | ||
| f"order); disable salt (--warmup-salt / warmup.salt: " | ||
| f"false) or use a text-prompt dataset" | ||
| ), | ||
| ) | ||
|
|
||
| def with_salt(self, rng: random.Random) -> "Dataset": | ||
| """Return a shallow copy of this dataset that salts each load_sample() call. | ||
|
|
||
| The returned dataset shares the same loaded data — no re-loading needed. | ||
| Each load_sample() call on the returned dataset prepends a unique hex salt | ||
| derived from rng to the prompt field, preventing KV-cache reuse. | ||
| derived from rng to the 'prompt' field, preventing KV-cache reuse. | ||
|
|
||
| Validates every sample first (see validate_saltable): a non-saltable | ||
| dataset raises here, before any load is issued. | ||
|
|
||
| Raises: | ||
| DatasetValidationError: if any sample cannot be salted. | ||
| """ | ||
| self.validate_saltable() | ||
| clone = copy.copy(self) | ||
| clone._salt_rng = rng | ||
| return clone | ||
|
|
||
| def _apply_salt(self, data: Any) -> Any: | ||
| """Prepend a unique salt to the prompt field of a sample dict.""" | ||
| def _apply_salt(self, data: dict[str, Any]) -> dict[str, Any]: | ||
| """Prepend a unique salt to the 'prompt' field. | ||
|
|
||
| with_salt() has validated every sample, so ``data`` is guaranteed to be a | ||
| dict with a str 'prompt' and no 'input_tokens'. | ||
| """ | ||
| assert self._salt_rng is not None | ||
| if not isinstance(data, dict): | ||
| return data | ||
| if "input_tokens" in data and "prompt" not in data: | ||
| self.logger.warning( | ||
| "salt=True: sample has 'input_tokens' but no 'prompt' — " | ||
| "salt cannot be applied to pre-tokenized input; KV-cache reuse may not be prevented" | ||
| ) | ||
| return data | ||
| if "input_tokens" in data and "prompt" in data: | ||
| self.logger.warning( | ||
| "salt=True: sample has both 'input_tokens' and 'prompt' — " | ||
| "salt applied to 'prompt' only; adapters that use 'input_tokens' " | ||
| "directly will still reuse the KV cache" | ||
| ) | ||
| if "prompt" not in data: | ||
| return data | ||
| prompt = data["prompt"] | ||
| salt = self._salt_rng.randbytes(8).hex() | ||
| if isinstance(prompt, str): | ||
| return {**data, "prompt": f"[{salt}] {prompt}"} | ||
| if isinstance(prompt, list) and prompt: | ||
| # Find the first text part at any index (image-first prompts place text at index 1+) | ||
| for i, part in enumerate(prompt): | ||
| if isinstance(part, dict) and part.get("type") == "text": | ||
| salted_parts = [ | ||
| *prompt[:i], | ||
| {**part, "text": f"[{salt}] {part['text']}"}, | ||
| *prompt[i + 1 :], | ||
| ] | ||
| return {**data, "prompt": salted_parts} | ||
| self.logger.warning( | ||
| "salt=True: multimodal prompt has no text part — " | ||
| "salt cannot be applied; KV-cache reuse may not be prevented" | ||
| ) | ||
| return data # unsupported prompt type — skip salting | ||
| return {**data, "prompt": f"[{salt}] {data['prompt']}"} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have 2 questions:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| def num_samples(self) -> int: | ||
| assert self.data is not None, "Dataset not loaded. Call load() first." | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have been caught earlier, but that is something to address -
exceptions.pyis very poorly designed and only contains bare name aliases for particular failure states.This construction of error message and
_salt_violationshould be part of the DatasetValidationError class:Ideally, reason should be some enum or object rather than a raw string, smth like