Fix bug in block manager's may_append #66
Conversation
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
|
Hi, thanks for your fix. I tried your fix and found an edge case that could lead to assert error. If the prompt length equals to block size, the last block hash will be not -1 (refer to the allocate function), triggering the assert error at line 117. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Hi nano-vllm team, this is Yue, nice to meet you, I'm just a fan of this repo and learning it! As I learn the code of block_manager, IIUC the current implementation of can_append and may_append is assuming seq already includes the upcoming token_id - this seems a bug to me as the upcoming token_id will be added in scheduler's postprocess which comes after the schedule(). I saw an existed PR fix the can_append() function, I'm creating this PR to fix the may_append() function.
I might be wrong if I miss any context, please correct me if I'm wrong :) Thanks for review!