-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Core] Add basic unit test for maybe_evict_cached_block #21400
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: Jialin Ouyang <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
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
The test setup for test_maybe_evict_cached_block
uses pool.blocks
directly, including the null_block
which is a special block that should not be cached or be part of the regular block pool logic. The test should only operate on non-null blocks by creating a pool with an extra block and then allocating the test blocks from the free queue. Also, using block.block_id
in assertions instead of hardcoding block IDs will make the test more resilient to future changes.
Signed-off-by: Jialin Ouyang <[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.
thx for adding the tests
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: qizixi <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: avigny <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: shuw <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: x22x22 <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: Paul Pak <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]>
…#21400) Signed-off-by: Jialin Ouyang <[email protected]>
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.Purpose
#21281 breaks the cache block eviction logic which was fixed by #21357 (Kudos to @simon-mo)
Introduce a unit test to at least cover some basic logic for _maybe_evict_cached_block.
Test Plan
Reject the error to ensure unit test failed.
Test Result
Test failed after locally reverted the fix PR.
(Optional) Documentation Update