[Store] Fix with_hard_pin failure in python API#1873
[Store] Fix with_hard_pin failure in python API#1873stmatengss merged 4 commits intokvcache-ai:mainfrom
with_hard_pin failure in python API#1873Conversation
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com> fix Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the Mooncake Store client-facing APIs with the C++ core’s ReplicateConfig.with_hard_pin capability by exposing the flag in bindings.
Changes:
- Expose
ReplicateConfig.with_hard_pinin the Python pybind11 module. - Extend the C API replicate config (
mooncake_replicate_config_t) and map it into the C++ReplicateConfig.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
mooncake-store/src/store_c.cpp |
Maps the C replicate config into the C++ ReplicateConfig, including with_hard_pin. |
mooncake-store/include/store_c.h |
Adds with_hard_pin to the public C API replicate config struct. |
mooncake-integration/store/store_py.cpp |
Exposes with_hard_pin to Python via pybind11. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct mooncake_replicate_config { | ||
| size_t replica_num; | ||
| int with_soft_pin; | ||
| int with_hard_pin; | ||
| const char **preferred_segments; |
There was a problem hiding this comment.
Adding with_hard_pin into mooncake_replicate_config changes the struct layout/size for the public C API. Existing callers compiled against the old header will pass a smaller/differently-laid-out struct, and the library will misinterpret preferred_segments as with_hard_pin (and shift the remaining fields), which can cause UB or unintended hard-pinning. Consider introducing a versioned config struct (e.g., mooncake_replicate_config_v2 + new entrypoints) or adding an explicit struct_size/version field so the callee can detect whether with_hard_pin is present; also update downstream language bindings that construct this struct by field name (e.g., the Rust wrapper currently uses a struct literal).
| config.replica_num = c_config->replica_num; | ||
| config.with_soft_pin = c_config->with_soft_pin != 0; | ||
| config.with_hard_pin = c_config->with_hard_pin != 0; | ||
| if (c_config->preferred_segments && |
There was a problem hiding this comment.
to_replicate_config() now unconditionally reads c_config->with_hard_pin. With the current C API change, any caller compiled against an older store_c.h (without the new field / different layout) can trigger incorrect reads (e.g., interpreting preferred_segments as the flag) and potentially crash or silently enable hard pinning. If backward compatibility is required, the C API needs a way to communicate the config struct version/size before this field is read.
| .def_readwrite("replica_num", &ReplicateConfig::replica_num) | ||
| .def_readwrite("with_soft_pin", &ReplicateConfig::with_soft_pin) | ||
| .def_readwrite("with_hard_pin", &ReplicateConfig::with_hard_pin) | ||
| .def_readwrite("preferred_segments", |
There was a problem hiding this comment.
The new Python-exposed with_hard_pin flag should be covered by the existing Python wheel tests that validate ReplicateConfig property access (e.g., default value and assignment), otherwise this regression can reappear unnoticed.
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option "with_hard_pin" to the ReplicateConfig structure. The changes include updating the C header definition, the conversion logic in the C++ implementation, and exposing the field via Python bindings. I have no feedback to provide.
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
LGTM |
Description
While PR #1728 introduced the
with_hard_pincapability to the C++ core, the corresponding Python interface was missing.Currently, attempting to configure
with_hard_pinvia the Python API results in the following error:Root Cause & Solution:
The
with_hard_pinattribute was not registered in the pybind11 module (store_py.cpp). This PR fixes the issue by explicitly exposingwith_hard_pinvia pybind11, aligning the Python API with the underlying C++ReplicateConfigstruct.Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-pg)mooncake-rl)Type of Change
How Has This Been Tested?
Checklist
./scripts/code_format.shbefore submitting.