-
-
Notifications
You must be signed in to change notification settings - Fork 12.1k
[CI] Fix Plugin Tests Tests #28413
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
[CI] Fix Plugin Tests Tests #28413
Conversation
LucasWilkinson
left a comment
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.
LGTM thanks!
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
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.
| 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})." | ||
| ) |
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.
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})."
)Signed-off-by: Robert Shaw <robertgshaw2@gmail.com>
Signed-off-by: Robert Shaw <robertgshaw2@gmail.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Signed-off-by: Robert Shaw <robertgshaw2@gmail.com>
Purpose
Test Plan
previously failed on this assert (block size was null)
Test Result
now passes
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.