Repository navigation
[fix] Omit missing fields in kv_batch_get instead of raising - #209
Conversation
Ascend#190 made `kv_batch_get` and `kv_batch_get_by_meta` raise `ValueError` when a `select_fields` entry is not ready in every requested key. Before, a field the existing keys do not hold was silently left out of the returned TensorDict. Restore that behavior: drop the check, so missing and unfinalized fields are omitted again. Missing keys still raise, and so does a `kv_batch_get_by_meta` `select_fields` entry absent from the given meta. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4a9f49aa5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| partition_id: Partition containing the keys | ||
| select_fields: Optional field(s) to retrieve. If None, retrieves all fields | ||
| select_fields: Optional field(s) to retrieve. If None, retrieves all fields. Fields | ||
| not ready in every key (missing, or not finalized) are omitted from the result. |
There was a problem hiding this comment.
When the requested keys have no common ready field—for example, two existing keys with disjoint field sets, or a new key whose only kv_update(..., finalize=False) field is hidden—kv_retrieve_meta returns a nonempty BatchMeta with no fields and is_ready == False. The deleted not_ready check is therefore not the only error path: the unchanged readiness check at lines 1189-1190 still raises ValueError, rather than omitting the unavailable selection as promised here. The new test masks this because its unrelated prompt field keeps the retrieved metadata ready, so behavior now depends on whether some unselected common field happens to exist.
Useful? React with 👍 / 👎.
Prepare TransferQueue 0.1.12 for release. This replaces #205. - Bump the package version from `0.1.12.dev0` to `0.1.12`. - Update the README and tutorials to cover shared fields, storage-side data parsing, data dumps, and KV locks. - Update the notebook examples for partial writes and reads. A batch read returns requested fields that are ready in every requested key, as documented in #209. Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
Problem
#190 changed what
kv_batch_getdoes when a requested key exists but a field inselect_fieldsdoes not. Before, the field was silently left out of the returned TensorDict. Now the call raisesValueError. This also affectskv_batch_get_by_meta, which goes through the same path. Callers that select a field only some keys may have produced, or one still being built withkv_update(finalize=False), started failing.Summary
not_readycheck that [feat] Support multi-stepkv_updatewith partial values hidden #190 added toasync_kv_batch_get.BatchMeta.select_fieldsdrops fields the meta does not hold, as before [feat] Support multi-stepkv_updatewith partial values hidden #190:kv_updatewith partial values hidden #190.kv_update(finalize=False)is not ready, so it stays hidden fromkv_batch_getuntil finalized, now without raising. This matches the README, which already sayskv_batch_getdoes not see such fields.Unchanged
ValueError.kv_batch_get_by_metastill raises when aselect_fieldsentry is not in theKVBatchMetapassed in. That check predates [feat] Support multi-stepkv_updatewith partial values hidden #190 and validates the caller's meta, not controller state.kv_updatewith partial values hidden #190 (finalize,kv_describe, dump/restore readiness, the create-on-missing behavior ofkv_update) is untouched.Compatibility
kv_batch_get/kv_batch_get_by_metawith aselect_fieldsentry that isn't ready in every key return a result without that field instead of raisingValueError. Callers that need to tell missing fields from unfinalized ones can usekv_describe.Tests
tests/e2e/test_kv_interface_e2e.py::test_unfinalized_updates_stay_hidden_until_finalizenow asserts that an unfinalized field is omitted, alone or alongside a ready field, and throughkv_batch_get_by_meta. It also covers a field that was never written.python -m pytest -q: 912 passed, 12 skipped. The 8 errors are all intests/test_yuanrong_storage_client_e2e.py, from the environment-specificTORCH_LIBRARYregistration error noted in [feat] Support multi-stepkv_updatewith partial values hidden #190.pre-commit runon the changed files passes.