-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[Refactor] Simplify BOS/EOS token handling #34435
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
Conversation
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
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.
Code Review
This pull request refactors the handling of BOS/EOS tokens by moving eos_token_id from Request to SamplingParams and centralizing the logic to get BOS/EOS token IDs into the Renderer. The changes are consistently applied throughout the codebase, including extensive updates to tests. This refactoring simplifies the code and removes unnecessary fallbacks, assuming tokenizer.eos_token_id is a required field. The implementation appears correct and improves code clarity. I have not found any issues with the changes.
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
|
/gemini review |
Signed-off-by: DarkLight1337 <[email protected]>
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.
Code Review
This pull request is a solid refactoring that simplifies token handling by moving eos_token_id from Request to SamplingParams and relocating BOS/EOS token retrieval methods to the Renderer. The changes are applied consistently across the codebase. However, I've identified a couple of issues related to the ignore_eos handling. The production scheduler logic in check_stop and the MockEngineCore in the test suite no longer respect the ignore_eos flag, which could lead to incorrect behavior and broken tests. I've provided suggestions to restore the previous, correct logic.
|
/gemini review |
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.
Code Review
This pull request is a well-executed refactoring that simplifies the handling of BOS/EOS tokens. Moving eos_token_id from Request to SamplingParams and centralizing token-related methods in the Renderer class are positive changes that improve code clarity and maintainability. The removal of unnecessary fallbacks for tokenizer.eos_token_id is also a good simplification. The changes have been applied consistently across the codebase, including extensive updates to the test suite, which demonstrates thoroughness. I have not found any issues of high or critical severity.
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Purpose
Request.eos_token_id -> SamplingParams.eos_token_id, which is only populated ifSamplingParams.ignore_eos is False.get_bos_token_idandget_eos_token_idfromInputPreprocessortoRenderer.tokenizer.eos_token_idas it's a required field now.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.