-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-41922: [CI][C++] Update Minio version #44225
Conversation
|
8c63110
to
aefba96
Compare
6fa9b17
to
16b9784
Compare
@github-actions crossbow submit -g cpp -g python -g wheel |
This comment was marked as outdated.
This comment was marked as outdated.
So, I don't understand why all Crossbow conda-based jobs above have failed with a weird CMake error, while the GHA conda-based jobs succeed. The error message doesn't ring a bell to me:
@kou Would you be able to advise? |
It seems that conda stopped setting https://github.com/ursacomputing/crossbow/actions/runs/11037675823/job/30659122209#step:7:1151
How about the following? diff --git a/ci/scripts/cpp_build.sh b/ci/scripts/cpp_build.sh
index bc2bba915f..ac0ba2bf72 100755
--- a/ci/scripts/cpp_build.sh
+++ b/ci/scripts/cpp_build.sh
@@ -34,7 +34,13 @@ if [ ! -z "${CONDA_PREFIX}" ] && [ "${ARROW_EMSCRIPTEN:-OFF}" = "OFF" ]; then
echo -e "===\n=== Conda environment for build\n==="
conda list
- export ARROW_CMAKE_ARGS="${ARROW_CMAKE_ARGS} -DCMAKE_AR=${AR} -DCMAKE_RANLIB=${RANLIB}"
+ if [ -n "${AR}" ]; then
+ ARROW_CMAKE_ARGS+=" -DCMAKE_AR=${AR}"
+ fi
+ if [ -n "${RANLIB}" ]; then
+ ARROW_CMAKE_ARGS+=" -DCMAKE_RANLIB=${RANLIB}"
+ fi
+ export ARROW_CMAKE_ARGS
export ARROW_GANDIVA_PC_CXX_FLAGS=$(echo | ${CXX} -E -Wp,-v -xc++ - 2>&1 | grep '^ ' | awk '{print "-isystem;" substr($1, 1)}' | tr '\n' ';')
elif [ -x "$(command -v xcrun)" ]; then
export ARROW_GANDIVA_PC_CXX_FLAGS="-isysroot;$(xcrun --show-sdk-path)" The GHA conda-based jobs use cached conda environment. So the conda change isn't affected for now. |
bd7f1fa
to
90c07b3
Compare
@github-actions crossbow submit -g cpp -g python |
Revision: 90c07b3 Submitted crossbow builds: ursacomputing/crossbow @ actions-299f61e3ea |
@github-actions crossbow submit -g wheel |
Your suggested fix looks good, @kou ! |
Revision: 90c07b3 Submitted crossbow builds: ursacomputing/crossbow @ actions-66b8892afd |
ci/conda_env_gandiva.txt
Outdated
clang>=11 | ||
llvmdev>=11 | ||
clang>=11,<19 | ||
llvmdev>=11,<19 |
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.
FYI: We can remove this by merging GH-44233.
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.
@kou Which one would you rather merge first?
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.
I've merged GH-44233.
I'll update this branch.
90c07b3
to
486bcc8
Compare
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.
+1
template <typename Error> | ||
void SaveBackend(const Aws::Client::AWSError<Error>& error) { | ||
S3Backend GetOrSetBackend(const Aws::Client::AWSError<Error>& error) { |
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.
GetOrSet
is a very weird name. What about SaveBackendIfAbsent
or SaveBackendFromErrorIfAbsent
?
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.
I don't know, I got the impression that it was a common name in caching patterns:
https://www.google.com/search?client=firefox-b-d&q=%22get_or_set%22
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.
But more importantly, I wanted to stress that this method is also used to retrieve the cached backend. The "Save" proposal doesn't do that (which is why I renamed the method in this PR).
@@ -1005,7 +1005,8 @@ TEST_F(TestS3FS, CreateDir) { | |||
AssertFileInfo(fs_.get(), "bucket/newdir/newsub/newsubsub", FileType::Directory); | |||
|
|||
// Existing "file", should fail | |||
ASSERT_RAISES(IOError, fs_->CreateDir("bucket/somefile")); | |||
ASSERT_RAISES(IOError, fs_->CreateDir("bucket/somefile", /*recursive=*/false)); | |||
ASSERT_RAISES(IOError, fs_->CreateDir("bucket/somefile", /*recursive=*/true)); |
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.
Someday the filesystem tests could be unified. I was very careful with directory semantics in the azure fs implementations. Testing all cases to ensure they behave like Linux system calls.
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.
Yes, we could open a new issue to improve the generic filesystem tests. Would you like to do that?
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.
Would you like to do that?
No. Because I know that the other filesystems diverge and these details have leaked into user code (i.e. they rely on the broken semantics).
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.
Fair enough. Usually there's a tension into enforcing consistent semantics accross filesystems, and trying to preserve performance when consistency implies more IO roundtrips to a remote host. If there are inconsistencies that are not motivated by performance, they should be investigated.
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.
S3 finally supports Conditional Writes. That means a lot of the check-before-create sequences you need to implement the filesystem semantics can be made much faster now.
https://aws.amazon.com/about-aws/whats-new/2024/08/amazon-s3-conditional-writes/
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.
Ok, I opened #44281 for it
850eb26
to
ca25668
Compare
@github-actions crossbow submit -g cpp -g python |
Revision: ca25668 Submitted crossbow builds: ursacomputing/crossbow @ actions-1eca55fdde |
We were using a rather old version of Minio for CI tests. Update the Minio version and ensure the S3FileSystem implementation passes all the tests with it.
ca25668
to
cce7a92
Compare
@github-actions crossbow submit example-python-minimal-build-fedora-conda |
Revision: cce7a92 Submitted crossbow builds: ursacomputing/crossbow @ actions-d81681459f
|
The CI failures are unfortunate but they are also unrelated. I'll merge. |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6b59098. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them. |
We were using a rather old version of Minio for CI tests.
Update the Minio version and ensure the S3FileSystem implementation passes all the tests with it.