Repository navigation
Check input + max_new_tokens against max_out_tokens in InferenceEngine.generate #8592
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
udsy19
wants to merge
4
commits into
deepspeedai:master
Choose a base branch
from
udsy19:fix/generate-total-length-check-3081
base: master
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 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
2137403
Check input + max_new_tokens against max_out_tokens in InferenceEngin…
udsy19 faeb85c
Also check max_length, and fix num_beams None crash blocking it
udsy19 b718dfd
Mirror transformers' max_new_tokens/max_length precedence in the leng…
udsy19 298f76f
Keep tensor_length in the max_length branch of the length guard
udsy19 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
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.
Thanks for picking both of these up so quickly. I ran the new commit in a clean container at
faeb85cd(python:3.11-slim, torch 2.14.0+cpu, transformers 5.17.0), and themax_lengthhalf over-rejects when a caller sets both budgets.max(total_length, max_length)takes the larger of the two, but transformers givesmax_new_tokensprecedence and ignoresmax_lengthwhen both are present, so the larger number is the one that will not be used. A call that fits now fails:The
num_beamsrepair makes this reachable from ageneration_configtoo, where it hit the TypeError first:GenerationConfig(max_length=4096)withmax_new_tokens=50againstmax_out_tokens=1024raises, and that generation is really 140 tokens.Mirroring the precedence instead of taking the max clears both, and your two new tests still pass on it:
I ran this on CPU against a stub module, so I have not exercised a real GPU generate path.
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.
Thanks for turning that around so fast. I ran b718dfd in a clean container (python:3.11-slim, torch 2.14.0+cpu, transformers 5.17.0) and the precedence part is right, but the snippet I handed you dropped
tensor_lengthfrom themax_lengthbranch, and that opens a gap I should have caught.max_lengthis a total, so when it is smaller than the input the guard now compares against a number below the input length and stops firing.It is not a silent overflow on a real model:
_validate_generated_lengthraisesValueError: Input length of input_ids is 2000, but max_length is set to 512immediately after. What goes missing is the engine's own error, the one that namesmax_out_tokens.Keeping the max inside the branch restores it:
On that, the two cases above raise again,
max_length=900over an 800 token input still passes through, and the test file's results are unchanged: 8 passed and 6 failed both with and without it, the 6 being a missing c++ fordeepspeed_shm_commplus the CUDA only cuda-graph test on this CPU box.Sorry for the round trip, that one is on my suggestion.