Skip MinLength logits processor construction when eos_token_id is negative

[titaiwangms]

What

Skip constructing the MinLengthLogitsProcessor when eos_token_id is negative.

A negative eos_token_id is the "no-EOS" sentinel used by greedy/sampling
generation (it defaults to -1). With no EOS token there is nothing to demote,
so a MinLength processor built with a negative eos can only ever be a guaranteed
no-op. This change guards its construction at the list level in
LogitsProcessorInitImpl (logits_processor.h), so we do not build a processor
that would do nothing.

if (parameters.min_length > 0 && parameters.eos_token_id >= 0) {
  ...  // add MinLength processor
}

Why

This is a small defense-in-depth / code-clarity improvement, not a behavior
change and not a correctness or security fix:

• For a valid eos_token_id >= 0, behavior is unchanged — the processor is
  still constructed and enforces the minimum length exactly as before.
• For a negative eos, the added guard skips a processor that would be a no-op
  anyway (SetScore already ignores negative token ids), so this is a minor
  performance and clarity optimization.
• It mirrors the existing conditional-adds in the same function (e.g.
  RepetitionPenalty), keeping the construction logic consistent.

CPU-only: the CUDA path is already inherently a no-op for a negative eos, so no
CUDA code is touched.

Tests

Adds 4 unit tests in min_length_logits_processor_test.cc (run via
onnxruntime_provider_test --gtest_filter='*MinLengthLogitsProcessorTest*'):

• SetScoreIgnoresNegativeTokenId — documents the pre-existing inline backstop
  that ignores negative token ids.
• ListInitSkipsProcessorForNegativeEosTokenId — drives LogitsProcessorList::Init
  with a negative eos and confirms a below-min-length run leaves scores unchanged
  (the processor is skipped as a guaranteed no-op).
• ListInitDemotesEosBelowMinLength — positive control / enforcement path: with a
  valid eos the processor is constructed and demotes the eos score below min length.
• ListInitLeavesScoresUnchangedAtMinLength — at the min length, no demotion.

The tests only reference exported (LogitsProcessorList::Init/Process) and
header-inline symbols so they link in both static and shared-library builds.

[Copilot]

Pull request overview

This PR improves robustness and clarity in the CPU generation logits-processor initialization by avoiding construction of a no-op MinLengthLogitsProcessor when eos_token_id is the negative “no EOS” sentinel (e.g., -1). It also adds focused unit tests to lock in both the processor behavior and the list-level construction guard.

Changes:

• Guard MinLengthLogitsProcessor construction in LogitsProcessorList::LogitsProcessorInitImpl behind eos_token_id >= 0.
• Add unit tests covering: negative EOS no-op, valid EOS demotion behavior, and list-level init guard behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
onnxruntime/contrib_ops/cpu/transformers/logits_processor.h	Adds an eos_token_id >= 0 guard to skip constructing a no-op MinLength processor for the negative sentinel.
onnxruntime/test/contrib_ops/min_length_logits_processor_test.cc	Adds unit tests for MinLengthLogitsProcessor behavior and for the new list-level construction guard.

[titaiwangms]

Review — multi-agent pass (readability, correctness, adversarial, spec/deep, integration)

Verdict: LGTM. The guard change is correct, safe, and behavior-neutral. Reviewers confirmed the "negative-eos MinLength is a no-op" premise directly: MinLengthLogitsProcessor::Process writes only via NextTokenScores::SetScore, which early-returns on token_id < 0 (logits_processor.h:31), and the processor has no other side effect or list-ordering dependency. >= 0 is the spec-correct threshold — eos_token_id defaults to -1 in all three parsers (greedy_search_parameters.cc:13, sampling_parameters.cc:11, beam_search_parameters.cc:24). BeamSearch/Whisper are unaffected (they hard-reject eos_token_id < 0 in Validate(), so the new conjunct is always true there). No CPU/CUDA divergence: the CUDA kernel already no-ops for a negative demote_token_id (word_id == demote_token_id never matches). New test file is auto-wired via the contrib_ops/*.cc CMake glob.

Worth addressing

• Major (test quality, non-blocking): the headline negative test doesn't actually guard the change. ListInitSkipsProcessorForNegativeEosTokenId asserts scores are unchanged with eos=-1 — but because SetScore already no-ops on negative ids (pre-existing, not in this diff), reverting the production guard leaves this test still passing. So it provides zero regression protection for the line it's meant to lock in, and the comment calling the guard "the sole observable behavior" is inaccurate. Either reframe the change honestly as a defensive/perf skip of a guaranteed no-op processor, or make the test discriminating (e.g. assert processor_list_ membership/size so removing the guard fails it). (via deep-reviewer)

• Minor: value-initialize the params struct. GreedySearchParameters parameters; (test l.40) leaves inherited scalar fields indeterminate. Safe today since Init only reads assigned fields, but brittle — a future guard reading another scalar could make these tests flaky. Use GreedySearchParameters parameters{};. (raised independently by code-reviewer and critical-reviewer)

Minor / nits (optional)

• PR description says "All 5 tests pass" but the file has 4 TEST(...) — reconcile the count. (readability)
• SetScoreIgnoresNegativeTokenId lives in the MinLengthLogitsProcessorTest suite but tests NextTokenScores::SetScore, not the processor — a *MinLengthLogitsProcessorTest* filter silently also runs it. Rename the suite or note it. (readability)
• kBatchBeamSize is assigned to parameters.batch_size; for the greedy/1-beam path it's really a batch size — consider kBatchSize. (readability)
• Trim the second sentence of the production comment ("Matches the conditional-add style…") and the multi-sentence per-test narration; the test names are already self-describing. (readability)

Follow-ups (out of scope)

• Sibling latent no-op: TemperatureLogitsProcessor. Same pattern as the one just fixed — with the default temperature == 1.0f the processor is constructed and added, but Process returns immediately for 1.0f. A follow-up could tighten the guard to temperature > 0 && temperature != 1.0f. (integration-reviewer)
• Schema note: eos_token_id is declared as a required INT attr in the GreedySearch/Sampling schema (contrib_defs.cc), yet the parser tolerates its absence via GetAttrOrDefault(-1). The -1 sentinel is a code convention, not schema-blessed — worth confirming the exporter is expected to emit eos_token_id=-1 for eos-less models. Pre-existing. (deep-reviewer)

Reviewed by a 5-agent team (Claude / GPT / Gemini). Findings deduplicated and prioritized by the lead; the Major was verified against logits_processor.h:31 before posting.