-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S3 support #426
S3 support #426
Changes from 100 commits
32130c2
4a5884c
804fd99
bbbe637
a752eee
ce4218a
5ba191d
5126b44
185e687
502f7cf
740a15d
7fbabdf
5b41e74
555903d
69f7fd9
39ca408
cce5b1b
1c601ad
551fe20
b0ff3d1
afb2dae
06c0311
504f15d
2de3486
5f07f2e
f391ad1
247ab4d
9adffc1
8b4436d
2287c9b
e830f62
bff52a4
f6e7ebc
3f8291a
2607ca1
6f3c25d
149d6be
98991b3
daac52f
ba6925c
c635519
b8bfef2
4c4b36e
e489bbe
73d9104
b4ac1d7
d9ba372
146efc0
b19a146
4cb54a9
379f521
8849d11
8c54a6c
bb0ad24
33b9227
2e5ff0b
378e9b6
496fe8d
ea2d93c
3bb3ce1
b9b2c44
fa4f298
c483b68
ce0c59a
bbb4042
f363f15
8129d17
37b294c
fb95490
2729dbf
ce08365
e800233
a60011a
0d68e49
72812cc
c4ccd9e
4f765d0
355d192
500681a
d2a49c9
bd83655
2687cc6
8fe82b9
fdb9a59
4e4389f
cebc0a8
61a5d50
d55030a
2123666
0c2f09b
bb445a3
4701b52
2bd411e
b40a0ad
7bcfe5b
2a9f934
b978323
00304e7
f9b49f2
0403686
0001265
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -64,11 +64,13 @@ requirements: | |||||
- rapids-build-backend >=0.3.0,<0.4.0.dev0 | ||||||
- scikit-build-core >=0.10.0 | ||||||
- libkvikio ={{ version }} | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We always need to be matching this line: https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/2e592a612d925988747cd6daa9e271328ceb3bfc/recipe/conda_build_config.yaml#L268
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for my ignorance, but why do we need an exact match? Naively, I would have thought that a lower-limit would be more flexible? Do all packages in conda-forge needs to maintain this pinning if they use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bdice, would it help if we remove the version requirement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First I'll explain the migration logic. Then I'll answer the questions you posed about other options. Migrations and why they're neededThe See the number of recent AWS-related rebuilds of libarrow, for instance: I'll ignore the In the past 12 months, there have been 6 rebuilds of libarrow for new versions of Every time that conda-forge migrates to a new version, we must use the same version so that RAPIDS (KvikIO specifically) can be installed alongside the latest conda-forge packages. Try this:
Note that Can we use a lower limit (or no version pinning)?If we were to use a lower limit (or no version pinning), kvikio would use the latest So what do we do?RAPIDS is not built with conda-forge infrastructure, but aims to be compatible with the conda-forge ecosystem. We do not benefit from conda-forge's automation for version migrations but we must track them in order to be compatible. (The conda-forge bots that "rebuild the world" do not rebuild RAPIDS recipes, but maybe we could create such a tool.) To remain compatible with conda-forge, we need to match the exact pinnings for core packages like Our best course of action for now is to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opened rapidsai/build-planning#100 to think more about conda-forge migration support in RAPIDS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, thanks for the explanation @bdice! I now see why you were hesitant from the get-go 😊 What if we move to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was actually thinking of using libcurl from the start since we only need the very basic S3 operations. I don’t think it will be too hard to implement and it will make Azure Blob Storage support straightforward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That might be a better option than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, I will give libcurl a try |
||||||
run: | ||||||
- python | ||||||
- numpy >=1.23,<3.0a0 | ||||||
- cupy >=12.0.0 | ||||||
- zarr | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not list a run dependency, this will be handled by run-exports: https://github.com/conda-forge/aws-sdk-cpp-feedstock/blob/f82d968670bdb9939ed7604a5fb7bb4885e2e6ba/recipe/meta.yaml#L14
Suggested change
|
||||||
# See https://github.com/zarr-developers/numcodecs/pull/475 | ||||||
- numcodecs !=0.12.0 | ||||||
- packaging | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -52,6 +52,7 @@ requirements: | |||||
{% else %} | ||||||
- libcufile-dev # [linux] | ||||||
{% endif %} | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Match conda-forge.
Suggested change
|
||||||
|
||||||
outputs: | ||||||
- name: libkvikio | ||||||
|
@@ -83,6 +84,7 @@ outputs: | |||||
{% else %} | ||||||
- libcufile-dev # [linux] | ||||||
{% endif %} | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this and add a |
||||||
test: | ||||||
commands: | ||||||
- test -f $PREFIX/include/kvikio/file_handle.hpp | ||||||
|
@@ -106,6 +108,7 @@ outputs: | |||||
- cuda-cudart-dev | ||||||
- libcufile-dev # [linux] | ||||||
{% endif %} | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this ignoring run exports? I'm confused overall by the changes in Perhaps this needs to use |
||||||
requirements: | ||||||
build: | ||||||
- cmake {{ cmake_version }} | ||||||
|
@@ -118,6 +121,7 @@ outputs: | |||||
- cuda-cudart-dev | ||||||
- libcufile-dev # [linux] | ||||||
{% endif %} | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
run: | ||||||
- {{ pin_compatible('cuda-version', max_pin='x', min_pin='x') }} | ||||||
{% if cuda_major == "11" %} | ||||||
|
@@ -127,6 +131,7 @@ outputs: | |||||
- cuda-cudart | ||||||
- libcufile # [linux] | ||||||
{% endif %} | ||||||
- aws-sdk-cpp>=1.11.267 | ||||||
about: | ||||||
home: https://rapids.ai | ||||||
license: Apache-2.0 | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,45 @@ | ||||||
# ============================================================================= | ||||||
# Copyright (c) 2024, NVIDIA CORPORATION. | ||||||
# | ||||||
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||||||
# in compliance with the License. You may obtain a copy of the License at | ||||||
# | ||||||
# http://www.apache.org/licenses/LICENSE-2.0 | ||||||
# | ||||||
# Unless required by applicable law or agreed to in writing, software distributed under the License | ||||||
# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||||||
# or implied. See the License for the specific language governing permissions and limitations under | ||||||
# the License. | ||||||
# ============================================================================= | ||||||
|
||||||
# This function finds aws-sdk-cpp and sets any additional necessary environment variables. | ||||||
function(find_and_configure_aws_sdk_cpp) | ||||||
include(${rapids-cmake-dir}/cpm/find.cmake) | ||||||
|
||||||
# Attempt to use find_package() - the patch is only needed if building from source | ||||||
set(CPM_USE_LOCAL_PACKAGES ON) | ||||||
|
||||||
rapids_cpm_find( | ||||||
AWSSDK 1.11.267 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must match conda-forge.
Suggested change
|
||||||
GLOBAL_TARGETS aws-cpp-sdk-s3 COMPONENTS S3 | ||||||
BUILD_EXPORT_SET kvikio-exports | ||||||
INSTALL_EXPORT_SET kvikio-exports | ||||||
CPM_ARGS | ||||||
GIT_REPOSITORY https://github.com/aws/aws-sdk-cpp.git | ||||||
GIT_TAG 1.11.393 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably needs to match the version above, too?
Suggested change
|
||||||
PATCH_COMMAND | ||||||
${CMAKE_COMMAND} | ||||||
-E | ||||||
env | ||||||
GIT_COMMITTER_NAME=rapids-cmake | ||||||
GIT_COMMITTER_EMAIL=rapids.cmake@rapids.ai | ||||||
git | ||||||
am | ||||||
--no-gpg-sign | ||||||
${CMAKE_CURRENT_LIST_DIR}/patches/aws-sdk-cpp/0001-Don-t-set-CMP0077-to-OLD.patch | ||||||
OPTIONS "BUILD_ONLY s3" "BUILD_SHARED_LIBS OFF" "ENABLE_TESTING OFF" "ENABLE_UNITY_BUILD ON" | ||||||
"USE_CRT_HTTP_CLIENT ON" | ||||||
) | ||||||
endfunction() | ||||||
|
||||||
find_and_configure_aws_sdk_cpp() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
From 7b24166a73e422e65b725ffcb0acd20ab493fac0 Mon Sep 17 00:00:00 2001 | ||
From: Kyle Edwards <kyedwards@nvidia.com> | ||
Date: Wed, 28 Aug 2024 15:32:07 -0400 | ||
Subject: [PATCH] Don't set CMP0077 to OLD | ||
|
||
--- | ||
CMakeLists.txt | 4 ---- | ||
1 file changed, 4 deletions(-) | ||
|
||
diff --git a/CMakeLists.txt b/CMakeLists.txt | ||
index c17ff8a07b1..b30bc81b6df 100644 | ||
--- a/CMakeLists.txt | ||
+++ b/CMakeLists.txt | ||
@@ -13,10 +13,6 @@ if (LEGACY_BUILD) | ||
"update the build flags as mentioned in README.md and set -DLEGACY_BUILD=OFF. " | ||
"The legacy support will be removed at 1.12.0 release.") | ||
|
||
- if (POLICY CMP0077) | ||
- cmake_policy(SET CMP0077 OLD) # CMP0077: option() honors normal variables. Introduced in 3.13 | ||
- endif () | ||
- | ||
get_filename_component(AWS_NATIVE_SDK_ROOT "${CMAKE_CURRENT_SOURCE_DIR}" ABSOLUTE) | ||
|
||
# Cmake invocation variables: | ||
-- | ||
2.34.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be defined as a part of
--cmake-args
, with no special--no-s3
flag? It is possible for users to specify the option via--cmake-args
. If the user gives a value there, we probably want to respect that with higher priority than defaulting to "ON" based on the absence of a--no-s3
flag.