kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #4294] [Store] Clarify repeated setup behavior in the Python binding #1075

New issue

Open

opened 2026-10-29 21:22:36 +00:00 by bsakaguchi · 4 comments

bsakaguchi commented

2026-10-29 21:22:36 +00:00

Owner

Originally created by @neverhook on GitHub (Sep 23, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4294

Problem

The Python wrapper's positional setup(), dictionary setup() and
setup_dummy() can replace an existing native client. There is no shared check
or documented contract for calling them again while the wrapper is active, or
after setup has failed. The replacement can also change which real/dummy client
the wrapper retains.

This came up in the review of #3335.
A guard had been added there, but it affects every Store user and is being
removed from the EGM change so the API decision can be reviewed separately.

Proposed follow-up

Decide whether repeated setup should be rejected until close() or whether
replacing an active client is supported with explicit teardown semantics.
Apply the chosen contract consistently across all three entry points, including
failed setup and transitions between real and dummy clients. Add Python tests
for those cases and document when a new setup is allowed.

This issue does not propose changing the native C++ repeated-setup contract or
adding retryable EGM cleanup. The generic mount/MR failure cleanup has its own
existing work in #2336.

Originally created by @neverhook on GitHub (Sep 23, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4294 ## Problem The Python wrapper's positional `setup()`, dictionary `setup()` and `setup_dummy()` can replace an existing native client. There is no shared check or documented contract for calling them again while the wrapper is active, or after setup has failed. The replacement can also change which real/dummy client the wrapper retains. This came up in [the review of #3335](https://github.com/kvcache-ai/Mooncake/pull/3335#discussion_r4080699889). A guard had been added there, but it affects every Store user and is being removed from the EGM change so the API decision can be reviewed separately. ## Proposed follow-up Decide whether repeated setup should be rejected until `close()` or whether replacing an active client is supported with explicit teardown semantics. Apply the chosen contract consistently across all three entry points, including failed setup and transitions between real and dummy clients. Add Python tests for those cases and document when a new setup is allowed. This issue does not propose changing the native C++ repeated-setup contract or adding retryable EGM cleanup. The generic mount/MR failure cleanup has its own existing work in #2336.

bsakaguchi commented

2026-10-29 21:22:37 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Sep 23, 2026):

Thanks for opening this issue, @neverhook!

Field	Value
Issue	#4294
GitHub user ID	`41352674`
Reporter	@neverhook

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Sep 23, 2026): Thanks for opening this issue, @neverhook! | Field | Value | |-------|-------| | **Issue** | #4294 | | **GitHub user ID** | `41352674` | | **Reporter** | @neverhook | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:22:37 +00:00

Author

Owner

@Id545 commented on GitHub (Sep 26, 2026):

I'd like to pick this up. Looking at store_py.cpp, init_real_client() swaps store_ before setup_real() runs, so a second setup() tears the old client down implicitly through its destructor (with the GIL held), and a failed setup leaves the half-initialized client as the active one.

My suggestion would be the simplest contract: reject setup() / setup(dict) / setup_dummy() while a client is active (clear error, ask for close() first), and only commit the new client to the wrapper once setup succeeds. Then add Python tests for repeated setup, failed setup, and real↔dummy transitions.

Does that match what you had in mind, or do you want replacement to stay supported?

@Id545 commented on GitHub (Sep 26, 2026): I'd like to pick this up. Looking at `store_py.cpp`, `init_real_client()` swaps `store_` before `setup_real()` runs, so a second `setup()` tears the old client down implicitly through its destructor (with the GIL held), and a failed setup leaves the half-initialized client as the active one. My suggestion would be the simplest contract: reject `setup()` / `setup(dict)` / `setup_dummy()` while a client is active (clear error, ask for `close()` first), and only commit the new client to the wrapper once setup succeeds. Then add Python tests for repeated setup, failed setup, and real↔dummy transitions. Does that match what you had in mind, or do you want replacement to stay supported?

bsakaguchi commented

2026-10-29 21:22:37 +00:00

Author

Owner

@neverhook commented on GitHub (Sep 30, 2026):

@Id545, I’d go with rejecting repeated setup until close() across positional setup, dictionary setup, and setup_dummy.

Please preserve the existing status-code convention and leave the active client unchanged on rejection. For a fresh attempt, construct the client locally and publish it only after setup succeeds. On failure or exception, reuse the existing native cleanup and leave the wrapper uninitialized so the caller can retry.

close() should clear both store_ and real_client_ and reset the mode; #3335 already touches this, so please coordinate that overlap. Keep close followed by fresh setup on the same wrapper supported. Please cover repeated setup, failure followed by retry, close followed by setup, and real/dummy transitions, and clarify the setup examples in the docs.

Let’s keep this scoped to the Python binding. Callers should serialize lifecycle calls and quiesce outstanding operations; concurrent replacement and native C++ repeated-setup semantics can remain separate.

@neverhook commented on GitHub (Sep 30, 2026): @Id545, I’d go with rejecting repeated setup until close() across positional setup, dictionary setup, and setup_dummy. Please preserve the existing status-code convention and leave the active client unchanged on rejection. For a fresh attempt, construct the client locally and publish it only after setup succeeds. On failure or exception, reuse the existing native cleanup and leave the wrapper uninitialized so the caller can retry. close() should clear both store_ and real_client_ and reset the mode; #3335 already touches this, so please coordinate that overlap. Keep close followed by fresh setup on the same wrapper supported. Please cover repeated setup, failure followed by retry, close followed by setup, and real/dummy transitions, and clarify the setup examples in the docs. Let’s keep this scoped to the Python binding. Callers should serialize lifecycle calls and quiesce outstanding operations; concurrent replacement and native C++ repeated-setup semantics can remain separate.

bsakaguchi commented

2026-10-29 21:22:37 +00:00

Author

Owner

@Id545 commented on GitHub (Sep 30, 2026):

Thanks @neverhook, I implemented that contract in kvcache-ai/Mooncake#4433 · [Store] Reject repeated setup in…:

setup() (positional and dict) and setup_dummy() return INVALID_PARAMS (-600) while a client is active, and leave that client unchanged. I picked -600 because RealClient::initAll_internal() already returns it for an already-initialized client. Happy to switch to another code if you prefer.
A new client is built locally and published only once its setup succeeds. On failure it goes through the existing tearDownAll(), with the GIL released for real clients, and the wrapper stays uninitialized so the caller can retry. The config dict is converted before any client is created, so a conversion error leaves nothing behind.
close() is the same hunk as in #3335, character for character, so whichever PR lands first, the other still merges cleanly on store_py.cpp. If you'd rather keep it in only one of them, tell me which.
Tests cover repeated setup through all three entry points, early and late setup failures followed by a retry, an exception during setup, close followed by setup, and real/dummy transitions. The successful dummy cases need a mooncake_client, so they run in run_tests.sh next to test_dummy_client.py. The rest runs in run_standalone_store_e2e.sh.
Docs: a lifecycle note under setup(), setup_dummy() and close(). The setup examples are now marked as alternatives.

While testing, I noticed that each successful setup/close cycle leaves about three file descriptors open. main shows the same count, so it isn't related to this change. I can open a separate issue if that helps.

@Id545 commented on GitHub (Sep 30, 2026): Thanks @neverhook, I implemented that contract in [kvcache-ai/Mooncake#4433 · [Store] Reject repeated setup in…](https://github.com/kvcache-ai/Mooncake/pull/4433): setup() (positional and dict) and setup_dummy() return INVALID_PARAMS (-600) while a client is active, and leave that client unchanged. I picked -600 because RealClient::initAll_internal() already returns it for an already-initialized client. Happy to switch to another code if you prefer. A new client is built locally and published only once its setup succeeds. On failure it goes through the existing tearDownAll(), with the GIL released for real clients, and the wrapper stays uninitialized so the caller can retry. The config dict is converted before any client is created, so a conversion error leaves nothing behind. close() is the same hunk as in #3335, character for character, so whichever PR lands first, the other still merges cleanly on store_py.cpp. If you'd rather keep it in only one of them, tell me which. Tests cover repeated setup through all three entry points, early and late setup failures followed by a retry, an exception during setup, close followed by setup, and real/dummy transitions. The successful dummy cases need a mooncake_client, so they run in run_tests.sh next to test_dummy_client.py. The rest runs in run_standalone_store_e2e.sh. Docs: a lifecycle note under setup(), setup_dummy() and close(). The setup examples are now marked as alternatives. While testing, I noticed that each successful setup/close cycle leaves about three file descriptors open. main shows the same count, so it isn't related to this change. I can open a separate issue if that helps.

bsakaguchi referenced this issue

2026-10-29 21:31:59 +00:00

[PR #1075] [MERGED] [Bug] fixed bug of master not using glog actually #1997

bsakaguchi referenced this issue

2026-10-29 21:33:38 +00:00

[PR #1370] [CLOSED] [TE] feat: zmq communicator based on coro_rpc #2204