Skip to content

[Wheel] Keep integer dtype for typed_ragged rows when a row is an empty list - #4611

Merged
zxpdemonio merged 1 commit into
kvcache-ai:mainfrom
Zhuoxi2000:fix-typed-ragged-empty-row-float
Oct 20, 2026
Merged

zxpdemonio merged 1 commit into
kvcache-ai:mainfrom
Zhuoxi2000:fix-typed-ragged-empty-row-float

Conversation

@Zhuoxi2000

@Zhuoxi2000 Zhuoxi2000 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Description

put_dataproto / get_dataproto changed integer list fields of non_tensor_batch to float64 if any row was an empty list. Rows [[1, 2], [], [2**53 + 1]] came back as [[1.0, 2.0], [], [9007199254740992.0]] with codec dtype float64. Both the element type and values above 2**53 changed. Without the empty row, the same field round-trips as int64. None rows are already excluded from inference.

Cause: np.asarray([]).dtype is float64. _choose_leaf_codec, in its list/tuple branch, and _encode_typed_ragged_values, when no dtype_hint is given, both pass that dtype to np.result_type along with the real rows.

Fix: a small helper, _typed_ragged_row_dtypes, leaves empty list/tuple rows out of promotion. It uses all rows only when every row is empty, so the all-empty case behaves as before. Empty np.ndarray rows still count, because their dtype is explicit. Both inference sites use the helper.

Behaviour after the change (main shown in brackets where it differs):

rows result dtype
[[1, 2], [], [2**53 + 1]] [[1, 2], [], [9007199254740993]] int64 (was float64, value changed)
[(1, 2), ()] [[1, 2], []] int64 (was float64)
[[True, False], []] [[True, False], []] via msgpack_ragged, same as without the [] row (was typed_ragged float64, [[1.0, 0.0], []])
[[1.5], []] unchanged float64
[np.array([], int32), np.array([1], int32)] unchanged int32
[[], []] unchanged unchanged

Overlap: rebased onto the relocated module from #3633 (python/mooncake/structured_object_store.py, tests in python/tests/store/). The Rust store copy added in #4254 (python/mooncake/store/rs/structured_object_store.py) has no typed_ragged inference, so nothing to port there. #4603 (integer map keys in msgpack_ragged decode, still open) touches a different function but adds its test near the same spot in test_structured_object_store.py, so the second of the two may have a trivial test-file conflict. I found no issue or PR that reports this bug.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake Conductor (mooncake-conductor)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Tests in python/tests/store/test_structured_object_store.py:

  • test_dataproto_helper_int_list_rows_with_empty_row_keep_int_dtype: [[1, 2], [], [2**53 + 1]] round trip keeps int values and int64 dtype.
  • test_choose_leaf_codec_ignores_empty_list_rows_for_dtype: _choose_leaf_codec picks typed_ragged with dtype int64 for int list/tuple rows with an empty row, and msgpack_ragged for [[True, False], []], the same as for [[True, False]].
  • test_dataproto_helper_bool_list_rows_with_empty_row_keep_bool_values: [[True, False], []] round trip uses msgpack_ragged and keeps bool values.

Test commands:

PYTHONPATH=python:mooncake-wheel pytest -q python/tests/store/test_structured_object_store.py \
  -k 'empty_row or ignores_empty_list_rows'
PYTHONPATH=python:mooncake-wheel pytest -q python/tests/store/test_structured_object_store.py \
  mooncake-wheel/tests/test_dataproto_catalog.py -k 'not with_real_store'

Test results:
Run on top of main at 19e0ab2 (Python 3.12, numpy 2.5.4, msgpack 1.2.3, pytest 9.1.1). On macOS the native mooncake._store extension is not built, so I ran with a small local sitecustomize stub that only provides ReplicateConfig for the import; the tests above use InMemoryStore, and the *_with_real_store tests are deselected.

  • Main plus the new tests: all 3 new tests fail ([9007199254740992.0] != [9007199254740993], 'float64' == 'int64', 'typed_ragged' == 'msgpack_ragged'); the two files give 3 failed, 150 passed, 27 skipped.
  • Only the _choose_leaf_codec change reverted: all 3 new tests fail. The codec test fails on 'float64' == 'int64'. The round trips fail too, because the decision's float64 dtype overrides the encoder's metadata, so int64 bytes decode as float64 ([5e-324, 1e-323] != [1, 2]) and the bool payload fails to decode (buffer size must be a multiple of element size).
  • With the fix: the two files give 153 passed, 27 skipped (main: 150 passed, 27 skipped). The skips are the same tests that skip on main.
  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done: round-tripped the inputs in the table above before and after the change

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh (no C/C++ changes)
  • I have run pre-commit on the files changed in this PR: ruff check 0.6.9, scripts/ci/check_changed_python_format.py origin/main, ruff format --check and codespell 2.2.6 all pass on both files.
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

AI assistance: this change was drafted with an AI coding assistant (Claude) and verified locally with the tests above. I reviewed every changed line and can answer for the change.

@zxpdemonio

Copy link
Copy Markdown
Collaborator

Overall, the implementation looks sound: filtering empty Python list/tuple rows while retaining empty ndarray dtypes and the all-empty fallback keeps codec selection and encoding consistent. I also verified the integer-precision case and the surrounding edge cases against current main.

Suggestion (non-blocking): Please add a regression case for [[True, False], []] (or directly assert that _choose_leaf_codec returns msgpack_ragged). The existing integer test would still pass if the _choose_leaf_codec change were removed, because _encode_typed_ragged_values independently recomputes the dtype. As a result, the current test does not protect the codec-selection branch changed by this PR.

The PR currently conflicts with main after the file relocation, so it will also need a rebase and a fresh CI run.

…ty list

NumPy gives an empty Python list a float64 dtype. Codec selection and
typed_ragged encoding promoted that dtype together with the other rows,
so a single [] row turned every integer row of a non_tensor_batch field
into float64: [[1, 2], [], [2**53 + 1]] round-tripped as
[[1.0, 2.0], [], [9007199254740992.0]].

Ignore empty list/tuple rows when inferring the common row dtype, and
fall back to all rows only when every row is empty. ndarray rows keep
their explicit dtype even when empty.

Tests cover the int round trip, the _choose_leaf_codec decision for int
and bool rows with an empty row, and a [[True, False], []] round trip
that keeps the msgpack_ragged codec and bool values.
@Zhuoxi2000
Zhuoxi2000 force-pushed the fix-typed-ragged-empty-row-float branch from 6eae0c2 to d0f37bd Compare October 18, 2026 04:35
@Zhuoxi2000
Zhuoxi2000 requested a review from ykwd as a code owner October 18, 2026 04:35
@Zhuoxi2000

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I rebased onto current main, so the change now lives in python/mooncake/structured_object_store.py and the tests in python/tests/store/test_structured_object_store.py.

I added two tests for the codec-selection branch:

  • test_choose_leaf_codec_ignores_empty_list_rows_for_dtype asserts _choose_leaf_codec returns typed_ragged with dtype int64 for [[1, 2], [], [2**53 + 1]] and [(1, 2), ()], and msgpack_ragged for [[True, False], []].
  • test_dataproto_helper_bool_list_rows_with_empty_row_keep_bool_values round-trips [[True, False], []] and checks the codec is msgpack_ragged and the values stay bool.

With only the _choose_leaf_codec change reverted, both new tests fail, and the codec test fails on 'float64' == 'int64'. They pass with the full change. One note: the existing int test also fails in that case, because _encode_structured_leaf merges decision.metadata over the encoder's metadata, so the int64 payload is decoded as float64. The direct codec test now makes that dependency explicit.

@github-actions github-actions Bot added the Store label Oct 18, 2026

@zxpdemonio zxpdemonio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@zxpdemonio
zxpdemonio merged commit 8952021 into kvcache-ai:main Oct 20, 2026
19 checks passed
zxpdemonio added a commit to zxpdemonio/Mooncake that referenced this pull request Oct 21, 2026
…20261021

* upstream/main: (507 commits)
  [Reshard] Plan Store KV reuse across TP, PP and host layouts (kvcache-ai#4762)
  [Store] Walk tenants through shard-backed cursors and build the removes on them (kvcache-ai#4796)
  [Store-RS][Bugfix] Match TENT Request Layout to the C ABI (kvcache-ai#4732)
  [TransferEngine] Recover auto GID after data-path retry exhaustion (kvcache-ai#4290)
  [Store] Add the namespace layer over the object route (kvcache-ai#4721)
  [Store] Fix silent loss of offload keys deferred to the ungrouped pool (kvcache-ai#3006) (kvcache-ai#3009)
  [Store] Remove redundant alignment headroom from DFS shard allocation (kvcache-ai#4753)
  [CI/Build] Gate ROCm hardware tests and bound nightly Docker builds (kvcache-ai#4750)
  [Bugfix][Store] Fix double-counting of segment OpLog metrics in OpLogApplier (kvcache-ai#4771)
  [Store] Skip hot-cache writeback for device-resident read destinations (kvcache-ai#4764)
  [Reshard] Add bounded in-flight execution for weight transfers (kvcache-ai#4628)
  [Store] Avoid racing with detached hot-cache fills during key scans (kvcache-ai#4334)
  [Store] HA: drain the master on SIGTERM/SIGINT before exit (part 1 of kvcache-ai#4710) (kvcache-ai#4745)
  TENT: report a notification the receiver could not take (kvcache-ai#4754)
  [Store] Reach object metadata through a per-key accessor contract (kvcache-ai#4717)
  [Store][Bugfix] Fix DFS sync client test retry failure (kvcache-ai#4781)
  [Wheel] Keep integer dtype for typed_ragged rows when a row is an empty list (kvcache-ai#4611)
  [Doc] Sync README news onto the docs homepage (kvcache-ai#4767)
  [CI] remove stale Torch version binding after PG JIT (kvcache-ai#4760)
  [TransferEngine] Add buffer-protocol and page-gathered batch transfer APIs (kvcache-ai#4763)
  ...
zxpdemonio added a commit to zxpdemonio/Mooncake that referenced this pull request Oct 21, 2026
…-ranges-20261021

* upstream/main: (394 commits)
  [Reshard] Plan Store KV reuse across TP, PP and host layouts (kvcache-ai#4762)
  [Store] Walk tenants through shard-backed cursors and build the removes on them (kvcache-ai#4796)
  [Store-RS][Bugfix] Match TENT Request Layout to the C ABI (kvcache-ai#4732)
  [TransferEngine] Recover auto GID after data-path retry exhaustion (kvcache-ai#4290)
  [Store] Add the namespace layer over the object route (kvcache-ai#4721)
  [Store] Fix silent loss of offload keys deferred to the ungrouped pool (kvcache-ai#3006) (kvcache-ai#3009)
  [Store] Remove redundant alignment headroom from DFS shard allocation (kvcache-ai#4753)
  [CI/Build] Gate ROCm hardware tests and bound nightly Docker builds (kvcache-ai#4750)
  [Bugfix][Store] Fix double-counting of segment OpLog metrics in OpLogApplier (kvcache-ai#4771)
  [Store] Skip hot-cache writeback for device-resident read destinations (kvcache-ai#4764)
  [Reshard] Add bounded in-flight execution for weight transfers (kvcache-ai#4628)
  [Store] Avoid racing with detached hot-cache fills during key scans (kvcache-ai#4334)
  [Store] HA: drain the master on SIGTERM/SIGINT before exit (part 1 of kvcache-ai#4710) (kvcache-ai#4745)
  TENT: report a notification the receiver could not take (kvcache-ai#4754)
  [Store] Reach object metadata through a per-key accessor contract (kvcache-ai#4717)
  [Store][Bugfix] Fix DFS sync client test retry failure (kvcache-ai#4781)
  [Wheel] Keep integer dtype for typed_ragged rows when a row is an empty list (kvcache-ai#4611)
  [Doc] Sync README news onto the docs homepage (kvcache-ai#4767)
  [CI] remove stale Torch version binding after PG JIT (kvcache-ai#4760)
  [TransferEngine] Add buffer-protocol and page-gathered batch transfer APIs (kvcache-ai#4763)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants