Repository navigation
[KV Offload] Pass request context - #39185
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a ReqContext dataclass to pass per-request parameters, such as kv_transfer_params, through the KV offloading pipeline. The OffloadingManager interface and its CPU implementation have been updated to include this context in lookup, prepare_load, and prepare_store methods. Feedback indicates that for full consistency and to support features like tenant-aware resource management, the ReqContext should also be added to the touch, complete_load, and complete_store methods. Furthermore, existing unit test mocks need to be updated to match the new method signatures to avoid TypeError exceptions.
| def lookup( | ||
| self, | ||
| block_hashes: Iterable[BlockHash], | ||
| req_context: ReqContext, |
There was a problem hiding this comment.
The ReqContext should also be added to the touch, complete_load, and complete_store methods. Since the stated goal of this PR is to support features like tenant routing and priority hints, these lifecycle management methods must also be context-aware. For example, a tenant-aware manager needs the context in complete_load to correctly release tenant-specific pins or quotas, and in touch to update the correct tenant's LRU state. Omitting the context here creates an inconsistent API that will likely require another breaking change soon.
|
|
||
| src_spec = self.manager.prepare_load(block_hashes) | ||
| req_context = ReqContext(kv_transfer_params=request.kv_transfer_params) | ||
| src_spec = self.manager.prepare_load(block_hashes, req_context) |
There was a problem hiding this comment.
Updating the prepare_load signature to require req_context will break the existing unit tests. Specifically, in tests/v1/kv_connector/unit/offloading_connector/utils.py, the MockOffloadingSpec initializes self.manager.prepare_load with a single-argument lambda (lambda block_hashes: ...). This will now raise a TypeError when called with two arguments in the scheduler. Please ensure the test mocks are updated to accommodate the new parameter.
|
This pull request has merge conflicts that must be resolved before it can be |
| @@ -304,7 +309,8 @@ def _get_reqs_to_store(self, scheduler_output: SchedulerOutput): | |||
| assert len(req.block_hashes) >= num_gpu_blocks | |||
|
|
|||
| new_offload_keys = group_state.offload_keys[start_block_idx:num_blocks] | |||
| store_output = self.manager.prepare_store(new_offload_keys) | |||
| req_context = ReqContext(kv_transfer_params=req.kv_transfer_params) | |||
There was a problem hiding this comment.
Can we re-use ReqContext by introducing a field in RequestOffloadState?
| """ | ||
| Prepare the given blocks to be offloaded. | ||
| The given blocks will be protected from eviction until | ||
| complete_store is called. | ||
|
|
||
| Args: | ||
| req_context: per-request context (e.g. kv_transfer_params). |
There was a problem hiding this comment.
Let's move this after keys to match the args order.
| @@ -114,7 +123,11 @@ def complete_load(self, keys: Iterable[OffloadKey]) -> None: | |||
| assert block.ref_cnt > 0, f"Block {key!r} ref_cnt is already 0" | |||
| block.ref_cnt -= 1 | |||
|
|
|||
| def prepare_store(self, keys: Iterable[OffloadKey]) -> PrepareStoreOutput | None: | |||
| def prepare_store( | |||
| self, | |||
There was a problem hiding this comment.
| self, | |
| self, |
| return ReqContext(kv_transfer_params=kv_transfer_params) | ||
|
|
||
|
|
||
| _CTX = make_req_context() |
There was a problem hiding this comment.
| _CTX = make_req_context() | |
| _EMPTY_REQ_CTX = make_req_context() |
orozery
left a comment
There was a problem hiding this comment.
Thanks! Please also make sure pre-commit passes following these changes.
| hits = self.manager.lookup(offload_keys[start_block_idx:]) | ||
| hits = self.manager.lookup( | ||
| offload_keys[start_block_idx:], | ||
| self._req_status[request.request_id].req_context, |
There was a problem hiding this comment.
| self._req_status[request.request_id].req_context, | |
| req_status.req_context, |
| @@ -249,7 +255,9 @@ def update_state_after_alloc( | |||
| assert len(request.block_hashes) // self.config.block_size_factor >= num_blocks | |||
| offload_keys = group_state.offload_keys[start_block_idx:num_blocks] | |||
|
|
|||
| src_spec = self.manager.prepare_load(offload_keys) | |||
| src_spec = self.manager.prepare_load( | |||
| offload_keys, self._req_status[request.request_id].req_context | |||
There was a problem hiding this comment.
| offload_keys, self._req_status[request.request_id].req_context | |
| offload_keys, req_status.req_context |
Signed-off-by: omerpaz95 <omerpaz95@gmail.com>
Signed-off-by: omerpaz95 <omerpaz95@gmail.com>
49aa8cf to
40f913b
Compare
…floading test lambdas for upstream req_context parameter (#1375) ## Root Cause Upstream vLLM PR vllm-project/vllm#39185 (commit `4353c9cb4a`) added a `req_context: ReqContext` parameter to `OffloadingManager.prepare_store()`, `prepare_load()`, and `lookup()` methods. The vllm-gaudi tests use mock `side_effect` lambdas and direct lambda assignments that only accepted the original single parameter (`block_hashes`/`keys`), but now receive two positional arguments. ## Fix Updated all 14 lambda signatures in `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` and 1 lambda in `utils.py` to accept the new `req_context` parameter (unused in test mocks). ## Files Changed - `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` — all `prepare_store.side_effect` lambdas: `lambda block_hashes:` → `lambda block_hashes, req_context:` - `tests/unit_tests/kv_offload/offloading_connector/utils.py` — `prepare_load` lambda: `lambda block_hashes:` → `lambda block_hashes, req_context:` ## Verification Verified on Gaudi3 HPU (g3 pod): - **Before fix**: 8/8 tests FAILED with `TypeError: lambda() takes 1 positional argument but 2 were given` - **After fix**: 8/8 tests PASSED (6.52s) Tests: `python -m pytest tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py -v` Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com>
…floading test lambdas for upstream req_context parameter (vllm-project#1375) ## Root Cause Upstream vLLM PR vllm-project/vllm#39185 (commit `4353c9cb4a`) added a `req_context: ReqContext` parameter to `OffloadingManager.prepare_store()`, `prepare_load()`, and `lookup()` methods. The vllm-gaudi tests use mock `side_effect` lambdas and direct lambda assignments that only accepted the original single parameter (`block_hashes`/`keys`), but now receive two positional arguments. ## Fix Updated all 14 lambda signatures in `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` and 1 lambda in `utils.py` to accept the new `req_context` parameter (unused in test mocks). ## Files Changed - `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` — all `prepare_store.side_effect` lambdas: `lambda block_hashes:` → `lambda block_hashes, req_context:` - `tests/unit_tests/kv_offload/offloading_connector/utils.py` — `prepare_load` lambda: `lambda block_hashes:` → `lambda block_hashes, req_context:` ## Verification Verified on Gaudi3 HPU (g3 pod): - **Before fix**: 8/8 tests FAILED with `TypeError: lambda() takes 1 positional argument but 2 were given` - **After fix**: 8/8 tests PASSED (6.52s) Tests: `python -m pytest tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py -v` Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com> Signed-off-by: Yeonsil Yoon <yeon.sil.yoon@intel.com>
…floading test lambdas for upstream req_context parameter (vllm-project#1375) ## Root Cause Upstream vLLM PR vllm-project/vllm#39185 (commit `4353c9cb4a`) added a `req_context: ReqContext` parameter to `OffloadingManager.prepare_store()`, `prepare_load()`, and `lookup()` methods. The vllm-gaudi tests use mock `side_effect` lambdas and direct lambda assignments that only accepted the original single parameter (`block_hashes`/`keys`), but now receive two positional arguments. ## Fix Updated all 14 lambda signatures in `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` and 1 lambda in `utils.py` to accept the new `req_context` parameter (unused in test mocks). ## Files Changed - `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` — all `prepare_store.side_effect` lambdas: `lambda block_hashes:` → `lambda block_hashes, req_context:` - `tests/unit_tests/kv_offload/offloading_connector/utils.py` — `prepare_load` lambda: `lambda block_hashes:` → `lambda block_hashes, req_context:` ## Verification Verified on Gaudi3 HPU (g3 pod): - **Before fix**: 8/8 tests FAILED with `TypeError: lambda() takes 1 positional argument but 2 were given` - **After fix**: 8/8 tests PASSED (6.52s) Tests: `python -m pytest tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py -v` Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com> Signed-off-by: bmyrcha <bartosz.myrcha@intel.com>
Signed-off-by: omerpaz95 <omerpaz95@gmail.com> Signed-off-by: Avinash Singh <avinashsingh.rcoem@gmail.com>
…floading test lambdas for upstream req_context parameter (#1375) ## Root Cause Upstream vLLM PR vllm-project/vllm#39185 (commit `4353c9cb4a`) added a `req_context: ReqContext` parameter to `OffloadingManager.prepare_store()`, `prepare_load()`, and `lookup()` methods. The vllm-gaudi tests use mock `side_effect` lambdas and direct lambda assignments that only accepted the original single parameter (`block_hashes`/`keys`), but now receive two positional arguments. ## Fix Updated all 14 lambda signatures in `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` and 1 lambda in `utils.py` to accept the new `req_context` parameter (unused in test mocks). ## Files Changed - `tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py` — all `prepare_store.side_effect` lambdas: `lambda block_hashes:` → `lambda block_hashes, req_context:` - `tests/unit_tests/kv_offload/offloading_connector/utils.py` — `prepare_load` lambda: `lambda block_hashes:` → `lambda block_hashes, req_context:` ## Verification Verified on Gaudi3 HPU (g3 pod): - **Before fix**: 8/8 tests FAILED with `TypeError: lambda() takes 1 positional argument but 2 were given` - **After fix**: 8/8 tests PASSED (6.52s) Tests: `python -m pytest tests/unit_tests/kv_offload/offloading_connector/test_scheduler.py -v` Signed-off-by: Paweł Olejniczak <pawelx.olejniczak@intel.com> Signed-off-by: Artur Fierka <artur.fierka@intel.com>
Signed-off-by: omerpaz95 <omerpaz95@gmail.com>
Signed-off-by: omerpaz95 <omerpaz95@gmail.com>
Purpose
Pass per-request context (
kv_transfer_params) through the CPU offloading manager interface.The
OffloadingManagerABC andCPUOffloadingManagernow accept aReqContextargument onlookup,prepare_load, andprepare_store.ReqContextwrapsrequest.kv_transfer_params(a free-form dict set viasampling_params.extra_args) and is constructed at each call site inOffloadingConnectorScheduler.This is wiring-only:
ReqContextis accepted but not yet consumed byCPUOffloadingManager.Structural changes:
ReqContextdataclass is defined invllm/v1/kv_offload/abstract.py(shared layer) rather thancpu/manager.py, so otherOffloadingManagerimplementations can use it without importing from the CPU-specific module.Test Plan
Test Result
Pre-commit (ruff, mypy, all hooks): all passed.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.