Add APIs for inferring remote endpoint type from URL. - #982
Conversation
In NVIDIA/cudf#22739, cudf / cudf-polars needed to infer the remote endpoint type kvikio would use for some URL. This adds public APIs to kvikio to do that.
|
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. |
|
/ok to test 2ca7f25 |
TomAugspurger
added
improvement
| if (probe_s3_connectivity && type == RemoteEndpointType::S3) { | ||
| // Check connectivity for the credential-based S3 endpoint and reuse this size in | ||
| // RemoteHandle::open to avoid a second HEAD request. | ||
| probed_nbytes = endpoint->get_file_size(); |
There was a problem hiding this comment.
Since the open() function is modified again, can we just cache the file size inside the endpoint as I suggested? That is, the first time the endpoint's file size gets called, HEAD (or whatever method necessary for the endpoint in question) is performed, result cached in a std::optional<std::size_t> _file_size, and subsequent file size query just reuses the cached value.
This way, this PR alone will reduce the cudf-polars per-file HEAD request to 1, if I'm not missing anything.
There was a problem hiding this comment.
Personally, I'm not comfortable making something like size part of the data model for RemoteHandle without also including something like an etag to ensure that we aren't operating on a stale file. By using size, cudf-polars is accepting that risk, but it's unavoidable for now; and it's at least no than what would currently happen if a file changes mid-query. But putting it into the kvikio API is a bit riskier I think.
And, at least from the cudf-polars side, we'll temporarily use kvikio to get the file size exactly once per file. But in the medium term we'll be able to get that information from cudf-polars; either way, we'll be able to provide it the size in all subsequent operations.
Finally, I don't think this would directly help cudf-polars since in our current design we don't store RemoteHandle instances. We'll just store the plc.io.types.SourceInfo and FileMetadata.
There was a problem hiding this comment.
Does this sound OK @kingcrimsontianyu? I'd like to merge this today if possible, so we can use it in NVIDIA/cudf#22739
| * | ||
| * @param url The URL of the remote file. | ||
| * @return The inferred endpoint type. | ||
| */ |
There was a problem hiding this comment.
I think we need to document the difference between this utility function and the open function, that given a URL valid for both S3 private and public endpoints, this function will always return the private endpoint type, and that it is not possible for us to disambiguate S3 private and public endpoints from a URL alone.
Then in the cudf PR NVIDIA/cudf#22739, we may document that known file size + URL without S3 credential is not supported.
There was a problem hiding this comment.
I added a note in b752f5a.
But I'd like to get away from "public" vs. "private" URLs entirely. It'd much prefer to just deal with some URL and an authorization method (possibly a chained authorization method that tries multiple).
kingcrimsontianyu
left a comment
kingcrimsontianyu
left a comment
There was a problem hiding this comment.
Lgtm!
Nit: suggested using structured binding in place of tie.
|
/merge |
3 participants
In NVIDIA/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::openwith the defaultAUTOmode.To do this, I've added a new public
infer_remote_endpoint_typemethod, along with a python binding.