Validate whisper beginning_timestamp_token_id bounds - #31636
Open
apsonawane wants to merge 2 commits into
Open
Conversation
- reject out-of-range beginning_timestamp_token_id for whisper logits processor once vocab size is known - add defense-in-depth runtime check in TimestampLogitsProcessor - add regression tests for invalid/valid boundary values
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds early validation for Whisper’s beginning_timestamp_token_id to prevent out-of-range indexing during timestamp logits processing, and extends unit tests to cover invalid/valid values.
Changes:
- Add bounds checks for
beginning_timestamp_token_idduring subgraph-parameter setup for Whisper generation. - Add a runtime guard in
TimestampLogitsProcessorto fail fast ifbeginning_timestamp_token_idis outside the logits width. - Add unit tests to ensure negative/out-of-range values are rejected.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxruntime/test/contrib_ops/beam_search_test.cc | Adds new unit tests covering invalid/valid Whisper beginning_timestamp_token_id values. |
| onnxruntime/contrib_ops/cpu/transformers/logits_processor.h | Adds runtime enforcement that beginning_timestamp_token_id is within the logits vector bounds. |
| onnxruntime/contrib_ops/cpu/transformers/beam_search_parameters.cc | Adds Whisper-specific validation of beginning_timestamp_token_id during SetSubgraphParameters. |
Suppressed comments (2)
onnxruntime/test/contrib_ops/beam_search_test.cc:67
- Like the previous Whisper validation test, initialize
parameters.vocab_sizeexplicitly to avoid using an indeterminate value inSetSubgraphParameters'svocab_size-override logic.
TEST(BeamSearchParametersTest, SetSubgraphParametersRejectsWhisperBeginningTimestampTokenIdEqualToVocabSize) {
contrib::transformers::BeamSearchParameters parameters;
parameters.model_type = contrib::transformers::IGenerationParameters::kModelTypeWhisper;
parameters.logits_processor = contrib::transformers::IGenerationParameters::kLogitsProcessorTypeWhisper;
parameters.beginning_timestamp_token_id = 128;
EXPECT_THROW(parameters.SetSubgraphParameters(128, 2, 4, 6), OnnxRuntimeException);
onnxruntime/test/contrib_ops/beam_search_test.cc:78
beginning_timestamp_token_id = 0is accepted by the test, but the Whisper timestamp logits processor assumesbeginning_timestamp_token_id > 0(it computes a max over the non-timestamp prefix[0, beginning_timestamp_token_id)). Use a positive in-range id here, and also setparameters.vocab_sizeexplicitly to avoid indeterminate default state.
TEST(BeamSearchParametersTest, SetSubgraphParametersAcceptsValidWhisperBeginningTimestampTokenId) {
contrib::transformers::BeamSearchParameters parameters;
parameters.model_type = contrib::transformers::IGenerationParameters::kModelTypeWhisper;
parameters.logits_processor = contrib::transformers::IGenerationParameters::kLogitsProcessorTypeWhisper;
parameters.beginning_timestamp_token_id = 0;
EXPECT_NO_THROW(parameters.SetSubgraphParameters(128, 2, 4, 6));
EXPECT_EQ(parameters.vocab_size, 128);
}
Comment on lines
+158
to
+163
| if (model_type == IGenerationParameters::kModelTypeWhisper && | ||
| logits_processor == IGenerationParameters::kLogitsProcessorTypeWhisper) { | ||
| ORT_ENFORCE(beginning_timestamp_token_id >= 0 && beginning_timestamp_token_id < vocab_size, | ||
| "beginning_timestamp_token_id is out of range, it is ", beginning_timestamp_token_id, | ||
| ", vocab_size is ", vocab_size); | ||
| } |
- require beginning_timestamp_token_id > 0 and < vocab_size - initialize vocab_size in new tests for deterministic branch behavior - update valid test case to beginning_timestamp_token_id=1
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request improves validation for the Whisper model's
beginning_timestamp_token_idparameter in both runtime and test code. The main focus is to ensure that invalid values for this parameter are caught early, preventing out-of-range errors during inference and processing.Validation improvements for Whisper timestamp token ID:
BeamSearchParameters::SetSubgraphParametersto enforce thatbeginning_timestamp_token_idis within the valid range[0, vocab_size)for Whisper models, throwing an error if not.TimestampLogitsProcessorto ensurebeginning_timestamp_token_id_is within bounds before processing logits.Unit test enhancements:
beam_search_test.ccto verify thatSetSubgraphParameterscorrectly rejects negative or out-of-rangebeginning_timestamp_token_idvalues and accepts valid ones.