Add a KvikIO-backed DiskResource for blocking disk spill I/O - #1186
Add a KvikIO-backed DiskResource for blocking disk spill I/O#1186nirandaperera wants to merge 1 commit into
DiskResource for blocking disk spill I/O#1186Conversation
f4febe5 to
215d9fc
Compare
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
@ me when this is ready for a full review (I still see plenty of failing CI here and it's behind main, unsure if it's ready yet).
Quick note... if this is merged, please also update RAPIDS dependency graph to reflect that, following these examples: NVIDIA/cuvs#2257 (review)
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| ); | ||
|
|
||
| /** | ||
| * @brief Durably synchronize file data to storage (fdatasync). |
There was a problem hiding this comment.
| * @brief Durably synchronize file data to storage (fdatasync). | |
| * @brief Durably synchronize file data to storage. |
There was a problem hiding this comment.
Let's not add implementation details on the docstrings.
| }; | ||
|
|
||
| /** | ||
| * @brief Spill directory from `disk_spill_dir` (`RAPIDSMPF_DISK_SPILL_DIR`). |
There was a problem hiding this comment.
| * @brief Spill directory from `disk_spill_dir` (`RAPIDSMPF_DISK_SPILL_DIR`). | |
| * @brief Spill directory from `disk_spill_dir`. |
There was a problem hiding this comment.
I don't think we need to list all possible ways to set a configuration, users should refer to the docs.
| auto file = std::make_unique<kvikio::FileHandle>( | ||
| path.string(), "w+", kvikio::FileHandle::m644, kvikio::CompatMode::AUTO | ||
| ); |
There was a problem hiding this comment.
| auto file = std::make_unique<kvikio::FileHandle>( | |
| path.string(), "w+", kvikio::FileHandle::m644, kvikio::CompatMode::AUTO | |
| ); | |
| auto const flags = std::filesystem::exists(path) ? "r+" : "w+"; | |
| auto file = std::make_unique<kvikio::FileHandle>( | |
| path.string(), flags, kvikio::FileHandle::m644, kvikio::CompatMode::AUTO); |
There was a problem hiding this comment.
As written, "w+" enables O_TRUNC, so every write() first truncates the path and a later pwrite() cannot retain bytes written by an earlier call. KvikIO supports "r+" for read/write access without truncation, so selecting "r+" when the spill file already exists (and "w+" only for initial creation) would preserve offset writes. I think the proposal above fixes nonzero offset writes.
Please also add a test that writes two distinct ranges to one path and verifies both remain intact.
| return options.get<std::filesystem::path>( | ||
| "disk_spill_dir", [](std::string const& value) { | ||
| if (value.empty()) { | ||
| return std::filesystem::temp_directory_path(); |
There was a problem hiding this comment.
Often this will be a ramdisk, which seems like a bad default.
There was a problem hiding this comment.
Why would this be a ramdisk? Wouldn't a ramdisk effectively be a proxy for spill-to-host?
There was a problem hiding this comment.
Ah nevermind, I misunderstood your statement. temp_directory_path() will often be a ramdisk, you're not saying users would often be a ramdisk. I agree with your statement.
4 participants
This adds a public, blocking
rapidsmpf::disk::DiskResourceon KvikIOCompatMode::AUTO(GDS when available) and keeps disk outsideMemoryType/Buffer/BufferResource.write/read/flushtake a path, pointer, size, device flag, and optional file offset; callers must sync device pointers before I/O.disk_spill_dir(RAPIDSMPF_DISK_SPILL_DIR); empty uses the system temp directory.Bufferround-trips, unaligned offsets, flush, and the spill-dir option.libkvikiois a public dependency oflibrapidsmpf(CMake, conda, DFG / pyproject).Closes #1183
Related to #1170