PERF/API: Specify datasource size to avoid HEAD requests for S3 endpoints - #22739
Merged
rapids-bot[bot] merged 35 commits intoJun 28, 2026
Merged
Conversation
…ints This updates libcudf and its usage of kvikio to avoid unnecessary HEAD requests in KvikIO to "open" a remote S3 file.
|
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. |
github-actions
Bot
added
libcudf
TomAugspurger
commented
Jun 1, 2026
TomAugspurger
added
improvement
TomAugspurger
marked this pull request as ready for review
June 1, 2026 19:48
TomAugspurger
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional per-path known file sizes across C++ types, datasource creation, remote handle opening, Python bindings, tests, docs, and CI packaging so callers can supply file size metadata and avoid HEAD requests when available. ChangesI/O Known File Size Support
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/tests/io/filepath_source_test.cpp (1)
49-73: 💤 Low valueConsider cleaning up the temporary test file.
The test writes to
std::filesystem::temp_directory_path() / "filepath_source_test.parquet"but doesn't explicitly remove it afterward. While this won't cause test failures, it could accumulate test artifacts over repeated runs.🧹 Suggested cleanup
Add cleanup after the test (or use RAII):
auto const result = cudf::io::read_parquet(read_opts); CUDF_TEST_EXPECT_COLUMNS_EQUAL(col, result.tbl->get_column(0).view()); + + std::filesystem::remove(filepath); }Alternatively, use a scoped temporary:
auto const temp_dir = cudf::test::get_temp_filepath("filepath_source_test.parquet");if such a helper exists in the test utilities.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/io/filepath_source_test.cpp` around lines 49 - 73, The test FilepathSourceTest::KnownSizePlumbsThroughMakeDatasources leaves a temp file at the path stored in the local variable filepath; ensure the file is removed after the test by deleting filepath (or use an RAII/temp-file helper such as get_temp_filepath if available) so the parquet artifact does not accumulate—add cleanup at the end of the TEST_F or replace filepath with a scoped temp helper when constructing cudf::io::parquet_writer_options and the sources vector.docs/cudf/source/cudf/io/io.md (1)
139-141: 💤 Low valueClarify the incorrect-size wording.
As phrased, it reads as if supplying an incorrect size is what suppresses HEAD requests. Any supplied
sizesuppresses HEAD; an incorrect one is what breaks reads.📝 Suggested reword
-Providing an incorrect size avoids the extra HEAD requests but will break footer reads and other -operations that depend on the true file length. Plain string paths in `SourceInfo` preserve the -previous behavior (size queried via KvikIO). +Supplying a `size` always suppresses the extra HEAD requests, but the value must be correct: an +incorrect size will break footer reads and other operations that depend on the true file length. +Plain string paths in `SourceInfo` preserve the previous behavior (size queried via KvikIO).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/cudf/source/cudf/io/io.md` around lines 139 - 141, Rewrite the sentence in the docs explaining size behavior so it clearly states that providing any size in SourceInfo (not specifically an incorrect size) suppresses HEAD requests, and that supplying an incorrect size will break footer reads and operations that depend on the true file length; update the paragraph mentioning "Plain string paths in `SourceInfo` preserve the previous behavior (size queried via KvikIO)" to follow this clarified wording so readers understand: any provided size disables HEAD, but an incorrect size causes failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/pylibcudf/pylibcudf/libcudf/io/types.pxd`:
- Around line 128-146: The Cython binding declares source_info(const
vector[filepath_source] &sources) but the C++ API is explicit
source_info(std::vector<filepath_source> sources); update the pxd to match the
C++ ctor by changing the declaration of source_info with filepath_source vector
to take the vector by value (vector[filepath_source] sources) with the same
exception specifier (except +libcudf_exception_handler), or alternatively add an
overload matching the C++ signature; ensure the symbol names source_info and
filepath_source and the filepath_sources() accessor remain unchanged.
---
Nitpick comments:
In `@cpp/tests/io/filepath_source_test.cpp`:
- Around line 49-73: The test
FilepathSourceTest::KnownSizePlumbsThroughMakeDatasources leaves a temp file at
the path stored in the local variable filepath; ensure the file is removed after
the test by deleting filepath (or use an RAII/temp-file helper such as
get_temp_filepath if available) so the parquet artifact does not accumulate—add
cleanup at the end of the TEST_F or replace filepath with a scoped temp helper
when constructing cudf::io::parquet_writer_options and the sources vector.
In `@docs/cudf/source/cudf/io/io.md`:
- Around line 139-141: Rewrite the sentence in the docs explaining size behavior
so it clearly states that providing any size in SourceInfo (not specifically an
incorrect size) suppresses HEAD requests, and that supplying an incorrect size
will break footer reads and operations that depend on the true file length;
update the paragraph mentioning "Plain string paths in `SourceInfo` preserve the
previous behavior (size queried via KvikIO)" to follow this clarified wording so
readers understand: any provided size disables HEAD, but an incorrect size
causes failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0cc67ef9-2151-4635-b9d9-7038a50a3f55
📒 Files selected for processing (13)
cpp/include/cudf/io/datasource.hppcpp/include/cudf/io/types.hppcpp/src/io/functions.cppcpp/src/io/utilities/datasource.cppcpp/tests/CMakeLists.txtcpp/tests/io/filepath_source_test.cppdocs/cudf/source/cudf/io/io.mdpython/pylibcudf/pylibcudf/io/__init__.pypython/pylibcudf/pylibcudf/io/types.pxdpython/pylibcudf/pylibcudf/io/types.pyipython/pylibcudf/pylibcudf/io/types.pyxpython/pylibcudf/pylibcudf/libcudf/io/types.pxdpython/pylibcudf/tests/io/test_source_sink_info.py
TomAugspurger
commented
Jun 1, 2026
| kvikio::RemoteEndpointType infer_remote_endpoint_type(std::string const& url) | ||
| { | ||
| if (kvikio::S3Endpoint::is_url_valid(url)) { return kvikio::RemoteEndpointType::S3; } | ||
| if (kvikio::S3PublicEndpoint::is_url_valid(url)) { return kvikio::RemoteEndpointType::S3_PUBLIC; } |
Contributor
There was a problem hiding this comment.
Here a URL from public S3 would still be inferred as private S3.
The challenge for S3 is that private and public endpoints follow the same URL pattern (S3PublicEndpoint::is_url_valid's implementation is the same with S3Endpoint::is_url_valid), and I think it is not quite possible to infer the type from URL alone without network I/O.
In KvikIO's open() method, given a S3-like URL (that is valid for both private and public S3), we first try to create a private S3 endpoint and access its file size, and in case of an exception, we fall back to the public S3 path.
Contributor
Author
There was a problem hiding this comment.
Would it be fair to say that there aren't really public and private URLs? Just URLs that happen to be anonymously accessible, and URLs that happen to require authorization, so they can't be disambiguated just by looking at the URL?
I think something like rapidsai/kvikio#951 would solve this pretty cleanly: you define a chain of credentials to attempt when making some HTTP request. So long as anonymous access (no credentials) is included in the chain then things will succeed.
For now, I think I'll remove the S3PublicEndpoint case, since IIUC it should be unreachable right now.
| * When `size` is set for a remote URL, the IO backend may skip querying the remote server for file | ||
| * size at open time. | ||
| */ | ||
| struct filepath_source { |
Contributor
There was a problem hiding this comment.
How about the name filepath_info?
Contributor
Author
There was a problem hiding this comment.
Hmm I was hoping for some kind of convention like a "source" is a thing that goes into a SourceInfo. Does that make sense?
If we change it to filepath_info, I'll want to change the python class to class FilepathInfo, which is a bit too close to SourceInfo for my taste.
This comment was marked as outdated.
This comment was marked as outdated.
rapids-bot Bot
pushed a commit
to rapidsai/kvikio
that referenced
this pull request
Jun 24, 2026
In rapidsai/cudf#22739, cudf / cudf-polars needed to infer the remote endpoint type kvikio would use for some URL, *without* making the HTTP request that'd come from `RemoteFile::open` with the default `AUTO` mode. To do this, I've added a new public `infer_remote_endpoint_type` method, along with a python binding. Authors: - Tom Augspurger (https://github.com/TomAugspurger) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Tianyu Liu (https://github.com/kingcrimsontianyu) URL: #982
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
I think there might still be some kind of build issue between here and rapidsmpf. https://github.com/rapidsai/cudf/actions/runs/28105062059/job/83225009485?pr=22739 is failure in |
Contributor
Author
|
b375237 fixed the build issue. Our libcudf_streaming wheel build needs to use the libcudf wheel from this PR, not nightly. |
bdice
approved these changes
Jun 26, 2026
kingcrimsontianyu
approved these changes
Jun 26, 2026
kingcrimsontianyu
left a comment
kingcrimsontianyu
left a comment
Contributor
There was a problem hiding this comment.
Lgtm!
Suggested adding a comment documenting the current limitation.
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Currently, reading, say, parquet metadata or data from a remote source with kvikio requires several HTTP requests. See #22667 (comment) and rapidsai/kvikio#974 for more, but a couple of these HTTP requests are
HEADrequests used to get the size of the file.This updates libcudf to provide a way to bypass those
HEADrequests when the file size is known in advance (#22734 is an example case where we already know the file sizes thanks to alistoperation). To do this, we expandSourceInfoto accept a (list of) simpleplc.io.types.FilepathSource(url, size=content_length)objects.For example,
read_parquet_metadataboth without and with file sizes:and
read_parquet:Note the lack of
kvikio::RemoteHandle::opencalls in the kvikio domain under thewith-sizeannotations. These came from the following script:Details
This is a necessary precursor to #22734.
Closes #22740