Repository navigation
[feat] add kv_update and field-level kv_clear - #182
Conversation
f735236 to
caf3753
Compare
[AI Review]Summary: I'd ask for changes before merging. The feature works and the PR's own tests pass, but there is a controller-metadata regression that the PR's main use case (releasing Permalinks below point to the branch head Reliability1. (High)
TransferQueue/transfer_queue/controller.py Lines 256 to 263 in caf3753 Controller-only repro: import torch
from transfer_queue.controller import DataPartitionStatus
t4 = {"dtype": torch.float32, "shape": (4,), "is_nested": False, "is_non_tensor": False}
empty = {"dtype": None, "shape": None, "is_nested": False, "is_non_tensor": True}
nested = {"dtype": torch.float32, "shape": None, "is_nested": True, "is_non_tensor": False,
"per_sample_shapes": {5: (3,), 6: (7,)}}
p = DataPartitionStatus(partition_id="train")
p.update_production_status([0, 1, 2, 3], ["pv"], {"pv": t4})
p.update_production_status([0], ["pv"], {"pv": empty}) # kv_empty on one key
print(p.field_metadata["pv"].to_batch_schema([1, 2, 3])) # keys 1..3: is_non_tensor=True, shape=None
print(p.update_production_status([5, 6], ["pv"], {"pv": nested})) # False: AssertionError inside FieldMeta.updateEnd to end on SimpleStorage (2 units): put This has three consequences:
This is the multimodal RL pattern the PR targets: the previous batch's The branch isn't specific to kv_update either. Any backend that writes a non-tensor value into a tensor field now takes it, and KV backends size their read buffers from TransferQueue/transfer_queue/storage/managers/base.py Lines 736 to 738 in caf3753 So "no change to put/get semantics for KV backends" doesn't strictly hold. Suggestion: don't express a per-key release through the column-level schema. One option is to treat a release as clearing the field for those samples:
This doesn't block training. TransferQueue/transfer_queue/controller.py Lines 720 to 763 in caf3753 It also means a released field can't be confused with a stored None. If keeping the field ready and returning None is the intended behavior, the controller's column metadata should at least be left untouched.
2. (Medium) Storage commits first, and a failed controller update is silently ignored The controller ACK carries TransferQueue/transfer_queue/storage/managers/base.py Lines 300 to 305 in caf3753 This predates the PR, but kv_update makes it much easier to hit. Besides the nested case above, a parser that changes dtype (e.g. 3. (Medium) A timeout doesn't mean the update wasn't applied The parser runs on the storage unit's request thread, the same thread that serves put, get, and the diagnostic probe. As a result:
4. (Low) Storage units decode tensors zero-copy from the received ZMQ frame: So samples sent to one unit in a single put are views of one buffer. Emptying part of a kv_batch_put batch frees nothing until the whole batch on that unit has been emptied.
The PR example uses per-key 5. (Low) The SSD two-phase put looks correct but belongs in its own PR I didn't find a problem in the logic: on failure only the files written by this call are removed, and the duplicate-index retention still holds across fields. Two notes:
Simplicity and maintainability6. The Today
7. The same argument validation is written four times: in TransferQueue/transfer_queue/storage/managers/simple_storage_manager.py Lines 544 to 553 in caf3753 Validating at the public entry point and at the RPC boundary is enough. _normalize_kv_update_args also returns parser and empty unchanged, and its dict→TensorDict block is copied from async_kv_put.
8. TransferQueue/transfer_queue/storage/managers/base.py Lines 809 to 825 in caf3753 Moving "requires the SimpleStorage backend" into the base message removes the override and four tests that only check its wording (Mooncake, Yuanrong, Ray, base KV). 9. The update RPC path mostly copies put. 10. Interface clarity11. 12. Two entry points do the same thing. 13. Only a single key is supported. 14. The name Suggested split
Verification at
|
|
Thanks for the detailed review. I reproduced the controller metadata regression, ignored controller ACK, unknown timeout outcome, and shared-allocation behavior before making the changes. Reliability
API and implementation simplification
Update atomicity remains per storage-unit request; a batch spanning multiple units is not a distributed transaction, and this is now documented. Verification
|
|
On tq.kv_clear(keys, partition_id) # unchanged: remove the whole key
tq.kv_clear(keys, partition_id, fields="pixel_values") # new: clear only these fields, keep the keyWhy change the semantics, not just the name Today A field-level clear would instead do the following:
Benefits:
What needs to change
Edge cases to pin down
A separate Either way, the shared-buffer caveat in |
|
Thanks — agreed that the semantics were the problem, not just the name. tq.kv_clear(keys, partition_id) # unchanged: remove the whole key
tq.kv_clear(keys, partition_id, fields="pixel_values") # clear only these fields, keep the keyController first. A new
Key mappings, tags, consumption status and indexes are left alone. Then storage.
One deviation from your sketch: the controller resolves fields from its own per-cell state instead of narrowing a Edge cases, as you listed:
The shared-buffer caveat in Tests:
Results:
I also rewrote the PR description to match the current API. |
cc28484 to
37a04a9
Compare
Cover argument normalization, unit-side parser/empty application, cross-unit field_schema assembly, controller non-tensor marking, and KV backend rejection. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
kv_update rewrites selected fields of an existing key by running a custom parser(old, new) on the SimpleStorage unit that holds the row, so the payload never travels to the client and back. kv_empty stores None instead, keeping the key and its production status. The unit computes every value before writing, so a failing parser leaves the row unchanged, and it reports only the shape it stored per sample; the manager assembles one batch-level field_schema ordered to match the notified indexes. FieldMeta now accepts a column turning non-tensor, so an emptied field stops being described as a tensor. KV-based backends reject the call. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
kv_update needs to run parser(old, new) where the value lives, which only SimpleStorage can do. The refusal used to list backend names in a string that would go stale as soon as another KV backend was added, and it never told the caller which backend they were actually on. Report the concrete manager class instead, and cover Mooncake, Yuanrong and Ray individually rather than only the shared base class. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
The unit tests stub out the transport, so they cannot show that a parser really travels to the storage unit or that a batch spanning several units reports its per-sample shapes back to the controller in the right order. This script exercises both against a live Ray cluster, alongside kv_empty, multi-field updates, non-tensor payloads, parser failure atomicity, argument validation, the async entry points, and updates issued from remote workers. It refuses to run when a controller already exists, so it can never attach to somebody else's deployment and clear their keys on the way out. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
With SSD offload on, put_data wrote, committed and cleaned up one field at a time. If writing a later field's files failed (for example a full disk), the earlier fields were already replaced and their old files deleted, yet the caller got an error. A plain put converges on retry, but kv_update parsers such as concat are not idempotent and the manager does not retry updates, so retrying applied the earlier fields twice. Write every field's files first and only then swap all references, update the SSD accounting and delete superseded files; any failure removes just the files this call wrote. The base put_data now checks every field's length before writing any, so the single commit cannot fail halfway either. apply_update's "a raise leaves storage unchanged" now holds for SSD write errors as well as parser errors. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
kv_clear(keys, partition_id, fields=...) now clears only the given fields and keeps the keys. The controller marks the cells as not produced, removes the samples from each field's FieldMeta and drops their backend metadata before storage deletes the values, so a cleared field reads as never written and later writes to the same field rebuild its schema normally. Tags, consumption status and global indexes are left untouched. Storage managers take an optional fields filter in clear_data: KV backends narrow the metadata with select_fields, and SimpleStorage units drop only those fields (unlinking their SSD files). kv_empty is removed. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
37a04a9 to
369a79c
Compare
Summary
Adds two operations on existing KV keys:
kv_update/kv_batch_updatemerge new values into fields that are already stored, usingmerge_fn(old, new). The merge runs on the SimpleStorage unit that holds each sample, so the stored payload never makes a round trip through the client.kv_clear(keys, partition_id, fields=...)clears only the given fields and keeps the keys, for example to releasepixel_valuesafter rollout while training still reads the rest of the sample. It works on every storage backend. Withoutfields,kv_clearbehaves as before.To make multi-field updates all-or-nothing on a unit, SSD-offloaded
put_datais now two-phase, which also affects plain multi-fieldput(see "SSD offload").Usage
kv_update: append the response to the promptFor a batch,
kv_batch_update(keys, partition_id, fields: TensorDict, merge_fn)takes a TensorDict whose batch size matcheskeys(nested tensors are allowed here). Both haveasync_variants.kv_clear(fields=...): release pixel values after rolloutIn a multimodal RL step, the rollout engine needs the raw
pixel_values, while training only needs the processed tensors. Once rollout is done, the pixel values only take up storage:After a field-level clear:
kv_putto the field writes it normally and marks it ready again.kv_clear. Clearing every field still keeps the key inkv_list; removing the key itself is stillkv_clearwithoutfields.Design
kv_update(data plane, SimpleStorage only)interface.async_kv_batch_update→client.async_update→AsyncSimpleStorageManager.update_data→ new ZMQ requestUPDATE_DATA→SimpleStorageUnit._handle_update→StorageUnitData.apply_update.ValueError. Nothing is created.put_data. A raisingmerge_fnor an incompatible result leaves that unit unchanged.field_schemainglobal_indexesorder (the controller readsper_sample_shapesby position) and notifies the controller. The client merges it into itsBatchMetawith the newBatchMeta.apply_field_schema(add_fieldsnow uses the same helper).notify_data_updatenow returns the ACK'ssuccess. On the update path, a rejected or missing ACK raisesRuntimeErrorstating that storage may already have committed and the merge must not be retried blindly.max_attempts=1, because a replay after a lost reply would fold the same value in twice. A timeout therefore has an unknown outcome: the unit may commit after the caller stops waiting. This is documented on the API.StorageManager.update_dataraisesNotImplementedError("kv_update is not supported by <ManagerClass>; it requires the SimpleStorage backend"). KV backends store each sample-field under its own key and have no hook to runmerge_fnwhere the value lives.Field-level
kv_clear(control plane first, then storage)interface.async_kv_clear(fields=...)→client.async_kv_clear_fields→ new controller requestKV_CLEAR_FIELDS→storage_manager.clear_data(metadata, fields=...).TransferQueueController.kv_clear_fieldsresolves the keys and captures the metadata of the affected cells, including custom backend metadata that KV backends need for deletion. ThenDataPartitionStatus.clear_fieldszeroes those production-status cells, calls the existingFieldMeta.remove_samples(which already falls back from nested to regular), and drops the per-field backend metadata. It does not touch key mappings, tags, consumption status or indexes, so there is no index-reuse race and noMARK_CLEARINGstep is needed.clear_datagains an optionalfieldsfilter. KV backends (Mooncake, Yuanrong, Ray) narrow the metadata withselect_fields. SimpleStorage forwardsfieldsinCLEAR_DATA, and units drop only those fields, unlinking their SSD files; the sample stays counted against unit capacity.kv_retrieve_meta, which only reports fields that every requested key has, so a field present on only some keys is still cleared on those keys.SSD offload: all-or-nothing multi-field put (#162)
HybridStorageUnitData.put_dataused to write, commit and clean up one field at a time, so a failure on a later field (for example, a full disk) left earlier fields replaced. It is now two-phase: write every field's SSD files first (on failure, unlink only the files this call wrote and re-raise), then swap all references and unlink superseded files. The baseStorageUnitData.put_datachecks every field's length before writing any.This also affects plain multi-field
putwith SSD offload on. The trade-off is a higher temporary peak: the old files plus all replacement files exist at once.docs/ssd_offload.mddocuments it.Shared backing memory
SimpleStorage slices dense batches without copying, so samples written by one batched put share an allocation. Replacing or clearing some of them does not return memory until every sibling is replaced or cleared.
docs/ssd_offload.mddocuments this alongside the existingdata_parsernote.Docs and tutorial
(async_)kv_update/(async_)kv_batch_updateadded,kv_clearnotes thefieldsoption, and theStorageManager.clear_datasignature is updated.tutorial/02_kv_interface.py: a new step that concatenates a field withkv_updateand then clears it withkv_clear(fields=...).docs/ssd_offload.md: shared backing memory and SSD headroom for atomic multi-field writes.Testing
python -m compileall -q transfer_queue tutorial testsand pre-commit (ruff, ruff-format, mypy) are clean.tests/e2e/test_kv_interface_e2e.pyon SimpleStorage with 2 units, sync and async: 64 passed, 2 skipped (the skips are KV-backend-only rejection tests).python -m pytest -q: 699 passed, 12 skipped, 8 errors. The errors are all intests/test_yuanrong_storage_client_e2e.pyat fixture setup, because the Yuanrongdatasystempackage is not installed locally.New and updated tests:
tests/test_kv_update.py: merge, atomicity on merge error or incompatible result, unproduced field rejection, decoded SSD-offloaded old values, atomicity when a later field fails to reach SSD, cross-unit schema assembly, KV-backend rejection naming the class.tests/test_storage_request_retry.py: the controller ACK status is returned; a rejected ACK after a storage commit raises; an update timeout is neither retried nor published.tests/test_controller_data_partitions.py: field-level clear keeps keys, tags, consumption status and other fields; the cleared field's schema for the remaining samples is unchanged; a later nested write to the same field succeeds;FieldMetais dropped once no sample holds the field.tests/test_simple_storage_unit.py: field-level clear in memory and on SSD (only that field's file is unlinked); all-or-nothing multi-field SSD put.tests/test_kv_storage_manager.py:clear_data(fields=...)deletes only those keys with their backend metadata.tests/e2e/test_kv_interface_e2e.py:TestKVUpdateE2E(update then clear a field, then nested writes; batch update across units; rejected arguments) andTestKVClearE2E::test_kv_clear_fields_keeps_keys_and_other_fields.Out of scope
kv_batch_get(select_fields=...)on a field that is not ready returns an empty result rather than raising. This is existing behavior.