[feat] Add RayStorageManager to backend#27
Conversation
Signed-off-by: Evelynn-V <liwenlin0223l@gmail.com>
Signed-off-by: Evelynn-V <liwenlin0223l@gmail.com>
Signed-off-by: Evelynn-V <liwenlin0223l@gmail.com>
Signed-off-by: Evelynn-V <liwenlin0223l@gmail.com>
Signed-off-by: Evelynn-V <liwenlin0223l@gmail.com>
Signed-off-by: Evelynn-V <liwenlin0223l@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a Ray-backed storage manager so TransferQueue can use the Ray/RDT KV backend via a dedicated RayStorageManager, aligning with the backend naming/initialization approach introduced in PR #26.
Changes:
- Introduces
RayStorageManagerregistered as theRayStorebackend type. - Exposes
RayStorageManagerthroughtransfer_queue.storageandtransfer_queue.storage.managersexports. - Adds a default
backend.RayStoreblock totransfer_queue/config.yaml(withclient_name: RayStorageClient).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
transfer_queue/storage/managers/ray_storage_manager.py |
New storage manager wrapper for Ray KV backend; validates/defaults client_name and delegates to KVStorageManager. |
transfer_queue/storage/managers/__init__.py |
Re-exports RayStorageManager from the managers package. |
transfer_queue/storage/__init__.py |
Re-exports RayStorageManager from the storage package. |
transfer_queue/config.yaml |
Adds default configuration section for backend.RayStore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RayStore: | ||
| client_name: RayStorageClient | ||
|
|
There was a problem hiding this comment.
Consider adding a short inline comment/header for this new RayStore block (matching the existing "# For SimpleStorage" section), and also update the earlier backend.storage_backend comment list to include RayStore so users discover the new backend option from the default config.
| def __init__(self, controller_info: ZMQServerInfo, config: dict[str, Any]): | ||
| client_name = config.get("client_name", None) | ||
|
|
||
| if client_name is None: | ||
| logger.info("Missing 'client_name' in config, using default value('RayStorageClient')") | ||
| config["client_name"] = "RayStorageClient" | ||
| elif client_name != "RayStorageClient": | ||
| raise ValueError(f"Invalid 'client_name': {client_name} in config. Expecting 'RayStorageClient'") | ||
| super().__init__(controller_info, config) |
There was a problem hiding this comment.
RayStorageManager introduces backend-specific config validation (defaulting/validating client_name) but there are no unit tests covering this behavior. Add a small test that constructs the manager (with controller connect mocked like other tests) to verify: (1) missing client_name is defaulted to RayStorageClient, and (2) a non-RayStorageClient value raises the expected ValueError.
|
look good to merge |
Background
In order to align with the backend mentioned in the PR#26, I have extracted the RayStorageManager to manage the RDT backend
Use Case