Skip to content

Respect max_gen_len for shorter prompts in SamplingSequenceGenerator - #1525

Open
kevin9327 wants to merge 1 commit into
facebookresearch:mainfrom
kevin9327:fix-sampling-max-gen-len
Open

kevin9327 wants to merge 1 commit into
facebookresearch:mainfrom
kevin9327:fix-sampling-max-gen-len

Conversation

@kevin9327

Copy link
Copy Markdown

What does this PR do?

Fixes #1078.

_AbstractSamplingSequenceGeneratorOp sets _max_seq_len to the longest prompt length plus max_gen_len and forces EOS only at that step. In a padded batch, a sequence whose prompt is shorter than the longest one could therefore generate max_gen_len + (longest prompt - its prompt) tokens.

This change forces EOS per sequence once it has generated max_gen_len tokens, using the per-sequence prompt lengths that are already tracked (and reordered) in _prompt_lens. Unpadded batches and the longest prompt behave exactly as before.

Tests

Added tests/unit/generation/test_sampling_generator.py, which runs the generator on a padded batch with prompt lengths 1 and 6 and max_gen_len=10, using a stub model that never predicts EOS.

  • Before the fix: the short-prompt hypothesis has 15 tokens (assert 15 == 10).
  • After the fix: both hypotheses have 10 tokens and end with EOS.

pytest tests/unit/generation, black, isort, flake8 and mypy pass on the changed files.

Check list

  • Did you write any new necessary tests?
  • Did you make sure to update the documentation with your changes? (no doc changes needed)
  • Did you update the CHANGELOG? (happy to add an entry if wanted)

AI assistance: this change was prepared with the help of an AI coding assistant. I reviewed the change and ran the tests above locally.

The generator stopped every sequence at the longest prompt length plus
max_gen_len, so a sequence with a shorter prompt in a padded batch could
generate up to (longest prompt - its prompt) extra tokens. Force EOS per
sequence once it has generated max_gen_len tokens.

Fixes facebookresearch#1078
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 17, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SamplingSequenceGenerator not respecting max_gen_len

1 participant