Skip to content

Conversation

@robertgshaw2-redhat
Copy link
Collaborator

@robertgshaw2-redhat robertgshaw2-redhat commented Nov 10, 2025

Purpose

Test Plan

  • CI (validated locally)
uv pip install -e ./plugins/vllm_add_dummy_platform pytest -v -s plugins_tests/test_platform_plugins.py

previously failed on this assert (block size was null)

Test Result

now passes


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.
Signed-off-by: Robert Shaw <robertgshaw2@gmail.com>
Copy link
Collaborator

@LucasWilkinson LucasWilkinson left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 correctly fixes a crash in plugin tests by ensuring an assertion related to Decode Context Parallelism (DCP) is only executed when DCP is active. This prevents a TypeError when block_size is None in non-DCP scenarios. I've added one suggestion to further improve robustness by adding an explicit check for block_size when DCP is enabled, which will provide a more informative error message in case of a misconfiguration.

Comment on lines 612 to 623
if self.parallel_config.decode_context_parallel_size > 1:
assert (
self.parallel_config.dcp_kv_cache_interleave_size
<= self.cache_config.block_size
and self.cache_config.block_size
% self.parallel_config.dcp_kv_cache_interleave_size
== 0
), (
f"Block_size({self.cache_config.block_size}) should be "
"greater than or equal to and divisible by dcp_kv_cache_interleave_size "
f"({self.parallel_config.dcp_kv_cache_interleave_size})."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

While this change correctly guards the assertion for when Decode Context Parallelism (DCP) is enabled, it could be made more robust. If decode_context_parallel_size > 1 but self.cache_config.block_size is None (e.g., due to a misconfigured custom platform plugin), the assertion will raise an unhelpful TypeError. It would be better to add an explicit check for block_size to provide a more informative error message.

 if self.parallel_config.decode_context_parallel_size > 1: assert self.cache_config.block_size is not None, ( "block_size must be set when using decode context parallelism (DCP)." ) assert ( self.parallel_config.dcp_kv_cache_interleave_size <= self.cache_config.block_size and self.cache_config.block_size % self.parallel_config.dcp_kv_cache_interleave_size == 0 ), ( f"Block_size({self.cache_config.block_size}) should be " "greater than or equal to and divisible by dcp_kv_cache_interleave_size " f"({self.parallel_config.dcp_kv_cache_interleave_size})." )
@LucasWilkinson LucasWilkinson enabled auto-merge (squash) November 10, 2025 20:04
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 10, 2025
Signed-off-by: Robert Shaw <robertgshaw2@gmail.com>
@LucasWilkinson LucasWilkinson merged commit 30700b1 into main Nov 10, 2025
50 checks passed
@LucasWilkinson LucasWilkinson deleted the fix-plugin-test branch November 10, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

3 participants