Conversation
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
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: ed9768257d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| done | ||
| done | ||
|
|
||
| python "${SCRIPT_DIR}/draw_figure.py" |
There was a problem hiding this comment.
When this documented SSD benchmark runs with a clean results/ directory, every generated filename ends in sample1mib, sample2mib, or sample4mib, but draw_figure.py accepts only small, medium, and large; it skips every new CSV and then fails at pd.concat([]). If older standard benchmark CSVs exist, it instead produces a chart from those stale, unrelated results. Thus the expensive benchmark either exits unsuccessfully or emits a misleading comparison rather than the advertised SSD chart; extend the plotter for these names/scenarios or invoke a dedicated SSD plotter.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I delete the draw command.
| @@ -0,0 +1,85 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Can we integrate the SSD test in existing test shell scripts?
There was a problem hiding this comment.
The current implementation uses a 1 MB threshold per sample, but the data sizes in the existing benchmark scripts are too small to trigger the offloading mechanism.
| storage.close() | ||
|
|
||
|
|
||
| def test_hybrid_storage_round_trips_raw_tensor_samples(tmp_path): |
There was a problem hiding this comment.
We may not need some of the tests here. Consider simplify the test.
| enabled: false | ||
| # Directory on a local NVMe SSD where offload files are written. | ||
| # Each storage unit creates its own sub-directory; files are removed on shutdown. | ||
| path: /tmp/tq_ssd_offload |
There was a problem hiding this comment.
/tmp usually is not a SSD path, but a tmpfs RAM space. This may explain the high performance in PR description. If this is the case, we have to rerun it.
There was a problem hiding this comment.
I have changed the default value to null, so users must explicitly provide a path. I also removed the code that handled the previous default value and added validation to ensure that at least the parent directory of the user-provided path exists.
ed97682 to
6b636de
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
6b636de to
988f095
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
988f095 to
b1d5412
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
| storage_unit_size: int | None = None, | ||
| ssd_config=None, | ||
| ssd_run_id: str | None = None, |
There was a problem hiding this comment.
It might be better to pass the simple storage related config in a single config param, e.g. https://github.com/Ascend/TransferQueue/blob/main/transfer_queue/storage/clients/mooncake_client.py#L56
| ssd_offload: | ||
| # Master switch. Set to true to enable SSD offload. | ||
| enabled: false | ||
| # Required when enabled. Its parent must exist on a local NVMe SSD; |
There was a problem hiding this comment.
we should allow the case when parent dir not exist.
| fd = os.open(temp_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) | ||
| try: | ||
| self._write_many(fd, [sample.payload]) | ||
| finally: | ||
| os.close(fd) |
There was a problem hiding this comment.
Maybe we can try to find some existing library that can help us to enable io_uring
There was a problem hiding this comment.
Thanks for the suggestion. For this PR, we intentionally use a small thread pool with regular POSIX file I/O to keep the initial implementation simple and avoid introducing additional dependencies. I plan to evaluate existing io_uring libraries in a follow-up PR, together with dedicated performance benchmarks.
|
Great work! I've already verified the effectiveness and stability of offloading in long runs locally. That said, there's one point worth raising: Symptom: After enabling SSD offload, storage unit RSS actually increases (+75% on the data plane in our 4-node test), even though payloads are correctly written to disk and everything functions normally. Diagnosis: Call Mechanism: The contiguous payload buffers generated by Fix: Set Suggestion: Consider adding an RSS measurement to |
b1d5412 to
0aec5e7
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
0aec5e7 to
2d6c916
Compare
CLA Signature Guide@xupinjie , thanks for your pull request. The following commit(s) are not associated with a signed Contributor License Agreement (CLA).
To sign CLA, click here. To check if your email is configured correctly, refer to the FAQs. Once you've signed the CLA or updating your email, please comment |
2d6c916 to
fc33c97
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
1 similar comment
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
@huniu20 Thanks for the detailed analysis. We addressed this in two ways: MALLOC_MMAP_THRESHOLD_ is now scoped to SSD-enabled storage actors, and SSD writes use per-sample zero-copy views instead of building concatenated payload buffers. We also added RSS monitoring to the dedicated two-node benchmark. It records aggregate storage-actor RSS before PUT and after PUT, GET, and CLEAR, together with active SSD bytes, and tracks post-CLEAR RSS across iterations. In our 1–4 GiB tests with 50% and 100% offload, active SSD bytes returned to zero after CLEAR and storage RSS did not progressively increase. |
6c53f72 to
1f436c2
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
|
@codex review |
| # transfer_queue_ssd_offload/ child, and leaves this directory intact on close. | ||
| # Example: /path/to/local/ssd | ||
| path: null | ||
| # Samples at or above this encoded size are stored on SSD. |
There was a problem hiding this comment.
We need to label the unit (KB/MB/GB...) in comment.
| # Required when enabled. TransferQueue creates this directory and any | ||
| # missing parents on every storage node, stores data under its | ||
| # transfer_queue_ssd_offload/ child, and leaves this directory intact on close. | ||
| # Example: /path/to/local/ssd |
There was a problem hiding this comment.
We can simplify the comments. Best fit in one line
| # SSD offload reduces the long-lived host-memory footprint by keeping large | ||
| # fields on local SSD. Small fields remain in memory. | ||
| # Each offloaded (field, global_index) sample owns one temporary file so | ||
| # CLEAR can reclaim its disk space immediately. |
There was a problem hiding this comment.
I suggest we provide a dedicate doc to describe the implementation details
There was a problem hiding this comment.
I add a new file docs/ssd_offload.md.
|
|
||
| TQ_STORAGE_POLLER_TIMEOUT = int(os.environ.get("TQ_STORAGE_POLLER_TIMEOUT", 5)) # in seconds | ||
| TQ_NUM_THREADS = int(os.environ.get("TQ_NUM_THREADS", 8)) | ||
| DEFAULT_SSD_OFFLOAD_THRESHOLD_BYTES = 1024 * 1024 |
There was a problem hiding this comment.
The default value is inconsistent with config.yaml
There was a problem hiding this comment.
Python：1024 * 1024 = 1048576
YAML：threshold_bytes: 1048576
They are both 1 MiB (1,048,576 bytes).
Signed-off-by: pinjie <pinjiex@nvidia.com>
1f436c2 to
5f2c7eb
Compare
CLA Signature Passxupinjie, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
Motivation
Large training samples can make the long-lived host-memory footprint of
SimpleStoragea capacity bottleneck as training scale and sequence length increase.Design
This PR adds an optional hybrid storage layer inside
SimpleStorageUnitwithout changing the public API, controller, or client behavior. Each(field, global index)sample is routed independently according to its actual stored payload size: samples at or above the internal 1 MiB threshold are written to node-local SSD, while smaller samples remain in host memory. The hybrid store preserves existing PUT, GET, CLEAR, overwrite, capacity, and checkpoint semantics; it reuses decoded contiguous buffers when possible and falls back to pickle for composite values. SSD files are temporary, isolated by run and storage unit, and removed during clear or graceful shutdown. The feature is disabled by default and configured throughssd_offload.enabledandssd_offload.path.E2E Performance
The following results are averages over four iterations with a batch size of 512 and eight fields. Throughput is reported in Gbit/s.
Test environment: 2 × Intel Xeon Platinum 8480C CPUs (224 cores), 10 × NVIDIA/Mellanox ConnectX-7 NDR 400 Gb/s InfiniBand adapters plus an Intel E810-C 100 GbE adapter, and 8 × KIOXIA CM6-R 3.84 TB NVMe SSDs (
KCM6DRUL3T84). 2 Node.