Skip to content

[Test] Cover cross-layer KV binding through patched reshape - #508

Merged
shipiyouniao merged 1 commit into
ovg-project:mainfrom
vmanvs:test/cross-layer-kv-binding
Oct 5, 2026
Merged

shipiyouniao merged 1 commit into
ovg-project:mainfrom
vmanvs:test/cross-layer-kv-binding

Conversation

@vmanvs

@vmanvs vmanvs commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

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_tensors entry point, using the existing fixture and the vLLM >=0.20 calling convention.

The test checks that:

  • Tensor-backed layers receive the expected pool objects across groups.
  • Runner-only sharing layers reference the exact same objects as their targets.
  • All expected layers appear in the returned mapping.

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:

  • Focused test file: 14 passed.
  • CPU test manifest: 296 passed.
  • In-memory regression checks: the test fails when reshape filtering or shared-layer aliasing is removed.

@shipiyouniao shipiyouniao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vmanvs

vmanvs commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

@shipiyouniao @RixinLiu thank you for approving this pr. is there any blocker to merge that you'd like me to address?

@shipiyouniao

Copy link
Copy Markdown
Collaborator

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.

@vmanvs

vmanvs commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

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.

@shipiyouniao
shipiyouniao merged commit 51f0c49 into ovg-project:main Oct 5, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants