Skip to content

Conversation

@mag1c-h
Copy link
Contributor

@mag1c-h mag1c-h commented Oct 16, 2025

Purpose

The C++ base class in Store already takes a raw pointer, now add the same pointer-based API to the Python base class to keep it extension-friendly.

Modifications

Add new pointer-based API to the Store Python base class.

Test

CI passed with new added/existing test.

@mag1c-h mag1c-h linked an issue Oct 16, 2025 that may be closed by this pull request
@mag1c-h mag1c-h requested review from Wwwzff, qyh111 and ygwpz October 16, 2025 07:30
@pyxyzc
Copy link
Contributor

pyxyzc commented Oct 16, 2025

@mag1c-h The semantics of the offset and size params kind of overlap here. Host–Storage transfers are already defined to operate on a page/block basis. Plz consider whether it really makes sense to expose this flexibility to the upper inference engines.

@mag1c-h
Copy link
Contributor Author

mag1c-h commented Oct 16, 2025

@mag1c-h The semantics of the offset and size params kind of overlap here. Host–Storage transfers are already defined to operate on a page/block basis. Plz consider whether it really makes sense to expose this flexibility to the upper inference engines.

@pyxyzc For the following reasons, we need to expose both offset and size parameters in the interface:

  1. Block semantics differ across storage layers:
    • On Host/Device, a "block" refers to a layer-block.
    • On Storage, a "block" contains all layers, each mapped to a different offset.
  2. GQA partitioning introduces per-tp offsets:
    When tp_range > 1 in GQA models, each tp starts from a distinct offset.
  3. Future-proofing for variable-size metadata:
    We may store profiling/characterization data where a single batch could contain entries of differing sizes.

@ygwpz
Copy link
Contributor

ygwpz commented Oct 17, 2025

do we really need two api here?

def load(self, block_ids: List[str], offset: List[int], dst_tensor: List[torch.Tensor]) -> Task
def fetch_data(self, block_ids: List[str], offset: List[int], dst_addr: List[int], size: List[int],) -> Task

i think fetch data is what we need final

@mag1c-h
Copy link
Contributor Author

mag1c-h commented Oct 17, 2025

do we really need two api here?

def load(self, block_ids: List[str], offset: List[int], dst_tensor: List[torch.Tensor]) -> Task
def fetch_data(self, block_ids: List[str], offset: List[int], dst_addr: List[int], size: List[int],) -> Task

i think fetch data is what we need final

They share the same impl, but keeping both API saves every caller a rewrite.

@ygwpz ygwpz merged commit d9b68aa into ModelEngine-Group:develop Oct 20, 2025
3 checks passed
@mag1c-h mag1c-h deleted the dev_store_intf branch October 20, 2025 03:18
flesher0813 pushed a commit that referenced this pull request Oct 24, 2025
* add store intf with tensor addr ptr

* fix interface doxy
flesher0813 added a commit that referenced this pull request Oct 24, 2025
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.

[Misc]: more interfaces are needed for ucmstore.py

4 participants