[feat] Make ds_max_workers configurable in YuanrongStorageClient - #157
[feat] Make ds_max_workers configurable in YuanrongStorageClient#157Chase-Rong wants to merge 1 commit into
Conversation
CLA Signature PassChase-Rong, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
![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: 196f904a77
ℹ️ 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".
|
|
||
| PUT_KEYS_LIMIT: int = 10_000 | ||
| GET_CLEAR_KEYS_LIMIT: int = 10_000 | ||
| # Default worker count for parallel serialization; override via config 'ds_max_workers'. |
There was a problem hiding this comment.
This comment merely restates that DS_MAX_WORKERS is a configurable default, rather than explaining why the constant exists, and it occupies only one line. Delete it instead of retaining narration that violates the repository requirement that comments be why-focused and 2–4 lines long.
AGENTS.md reference: AGENTS.md:L22-L26
Useful? React with 👍 / 👎.
| raise ValueError("Missing or invalid 'worker_port' in config") | ||
|
|
||
| ds_max_workers = config.get("ds_max_workers", self.DS_MAX_WORKERS) | ||
| if not isinstance(ds_max_workers, int) or ds_max_workers < 1: |
There was a problem hiding this comment.
When a YAML/config override supplies ds_max_workers: true, Python treats True as an int, so this validation accepts it even though the option is documented as a positive integer. batch_encode_into then evaluates True <= 1 and silently uses serial packing instead of reporting the invalid configuration; explicitly exclude bool or require type(ds_max_workers) is int.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
| enable_yr_npu_transport: false | ||
| # Number of worker threads used to serialize objects in parallel before storing | ||
| # them via the general KV client. 1 disables parallelism (serial packing). | ||
| ds_max_workers: 1 |
There was a problem hiding this comment.
Please also reflect in performance test config
There was a problem hiding this comment.
Please also reflect in performance test config
done
DS_MAX_WORKERS (worker threads for parallel serialization in GeneralKVClientAdapter.mset_zero_copy) was a hardcoded class constant. Read it from the Yuanrong backend config instead, keeping 1 (serial packing) as the default, and validate that it is a positive integer. Signed-off-by: lolo-pop <lolopop.rong@gmail.com>
196f904 to
72807a6
Compare
CLA Signature PassChase-Rong, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
3 participants
What does this PR do?
Makes the parallel-serialization worker count of Yuanrong's general KV client (
GeneralKVClientAdapter.DS_MAX_WORKERS) configurable via the Yuanrong backend config instead of a hardcoded class constant.Test