Repository navigation
[Test] Cover cross-layer KV binding through patched reshape - #508
Conversation
shipiyouniao
left a comment
There was a problem hiding this comment.
LGTM at ef58cee. Calling the patched reshape entry point closes a useful gap: the old test could pass even if that entry point stopped applying the helpers.
I reran the focused file (14 passed) and CPU manifest (296 passed) on Linux/Python 3.11. I also tried four in-memory mutations of the production methods: removing the runner-only filter, removing alias binding, swapping pool indices, and bypassing the patch. The new test failed for the expected reason in all four cases. All 11 CI checks are green.
This is a test-only improvement; these results do not claim new GPU or real-vLLM coverage. @RixinLiu, ready for your final look. I haven't merged it.
|
@shipiyouniao @RixinLiu thank you for approving this pr. is there any blocker to merge that you'd like me to address? |
|
Thanks for checking, and sorry for the wait. I don't have any further changes to request from you right now. We're currently focused on vLLM 0.29 compatibility and getting the runtime fixes needed for the release merged, so this may take a little longer. That doesn't make this test coverage less important. It catches regressions that the helper-only tests missed; the delay is about sequencing the release work, not a concern with your contribution. Thanks for your patience. |
|
Thank you for the clarification. Would be more than happy to help with the current roadmap and see if there's anything else I could contribute towards 0.29 compatibility. |
The existing sharing test reconstructs the pool-binding loop and calls the alias helper directly. It can therefore pass even if the patched reshape method stops filtering runner-only layers or applying shared-layer aliases.
Replaced that manual loop with a call through the actual patched
_reshape_kv_cache_tensorsentry point, using the existing fixture and the vLLM >=0.20 calling convention.The test checks that:
This strengthens regression coverage for #472. No production code changes; the test uses fake pools and does not exercise GPU allocation or real vLLM.
Validation: