Repository navigation
[Bugfix][KV Connector][ROCm] MoRIIO READ: skip zeroing READ destinations for mixed-precision caches #62313
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
[Bugfix][KV Connector][ROCm] MoRIIO READ: skip zeroing READ destinations for mixed-precision caches #62313
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1361,18 +1361,15 @@ def build_connector_meta( | |||||||||||||
| local_block_ids=block_ids, | ||||||||||||||
| kv_transfer_params=kv_params, | ||||||||||||||
| ) | ||||||||||||||
| if ( | ||||||||||||||
| self.mode == MoRIIOMode.READ | ||||||||||||||
| and self._has_mamba | ||||||||||||||
| and scheduler_output.new_block_ids_to_zero | ||||||||||||||
| ): | ||||||||||||||
| # Hybrid models zero recycled attention pages that held Mamba state. | ||||||||||||||
| # Host-submitted READs overwrite these pages and can race zeroing. | ||||||||||||||
| if self.mode == MoRIIOMode.READ and scheduler_output.new_block_ids_to_zero: | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this filter now runs on every READ-mode step for mixed-precision models, including the prefill side where
Suggested change
|
||||||||||||||
| # Mamba and mixed-precision caches zero new pages on the compute | ||||||||||||||
| # stream. Host-submitted READs overwrite these pages and can race it. | ||||||||||||||
| # Hybrid READ metadata puts the aligned attention pages first. | ||||||||||||||
| read_dst_block_ids = { | ||||||||||||||
| b | ||||||||||||||
| for _, block_ids in self._reqs_need_recv.values() | ||||||||||||||
| for b in block_ids[0] | ||||||||||||||
| for group in (block_ids[:1] if self._has_mamba else block_ids) | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is the condition now overly loose? IIUC zeroing happens either if it's mamba or if using mixed precision kv. Can we check that instead so it aligns full with Ideally we would use needs_kv_cache_zeroing directly so we don't have to mirror the check as that will cause drifts in the future, not sure if that's possible though |
||||||||||||||
| for b in group | ||||||||||||||
| } | ||||||||||||||
| scheduler_output.new_block_ids_to_zero = [ | ||||||||||||||
| b | ||||||||||||||
|
|
||||||||||||||
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.
Nit: the docstring says these models "zero pages in every READ group", but the test asserts the opposite (READ pages are dropped from the zeroing list).