-
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-40079: [CI][Packaging] Enable Azure in more tests and builds #40080
GH-40079: [CI][Packaging] Enable Azure in more tests and builds #40080
Conversation
|
Most of the CI failures are actually reproducible on I will need to investigate the AVX2 build failure. |
ci/docker/conda-cpp.dockerfile
Outdated
# Azurite requires npm | ||
RUN export DEBIAN_FRONTEND=noninteractive && \ | ||
apt-get update -y -q && \ | ||
apt-get install -y -q npm \ | ||
&& apt-get clean \ | ||
&& rm -rf /var/lib/apt/lists/* |
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 we install npm (Node.js) by conda?
Test is https://github.com/apache/arrow/actions/runs/7902101938/job/21566998998?pr=40080#step:6:6540
arrow/cpp/src/arrow/filesystem/azurefs_test.cc Lines 286 to 291 in 2422994
Important backtrace: https://github.com/apache/arrow/actions/runs/7902101938/job/21566998998?pr=40080#step:6:6568
arrow/cpp/src/arrow/filesystem/azurefs.cc Lines 184 to 185 in 2422994
|
Pinning to same versions we get from vcpkg solves some of the CI failures. I think the only remaining failures are ones that also exist on |
Does it mean that we may need to improve our Azure filesystem implementation to use more recent azure-sdk-for-cpp? (If it's needed, we should work on it as a separated task.) Anyway, could you add a comment why we need to pin azure-sdk-for-cpp?
Could you open an issue for it so that we can work on it as a separated task? |
Created an issue for the python 3.9 GCS testbench issue #40112 For the segfault it looks like going from and it looks like 1.11.0 has been marked as broken on the conda website If I understand this correctly it seems to be a conda issue not an Azure SDK or an Arrow problem. |
I'm going to call this ready for review but I'm not sure if we can merge it while #40112 is un-resolved. |
Hmm.... it looks like the azure tests are slightly flaky. I just changed a comment and the new CI run has an azure failure.
It looks like azurite returned an invalid response. I've run this multiple times locally but so far have not been able to reproduce. |
69ed3a5
to
7ed0910
Compare
Retrying got the same error in a different test case on a different build https://github.com/apache/arrow/actions/runs/7951594516/job/21705210845?pr=40080 |
I think #40120 should resolve the Python 3.9 and GCS testbench problem. I also found an example of the azure tests flaking on I created a separate issue to fix this flakiness #40121 |
7ed0910
to
7c27978
Compare
31e4920
to
5e3f846
Compare
ci/conda_env_cpp.txt
Outdated
@@ -16,6 +16,12 @@ | |||
# under the License. | |||
|
|||
aws-sdk-cpp=1.11.68 | |||
# There is a problem with the 1.11.0 conda release of azure-core-cpp https://github.com/conda-forge/admin-requests/pull/911 |
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 am not sure that is a relevant link. AFAIU there is not really a problem with the 1.11.0 conda package itself. It's that conda did not pin azure-core-cpp in other packages (eg building tiledb with azure-core-cpp against 1.10, and then running that in an env with 1.11 installed, then we got segfaults).
But for the purpose of our CI, we are both building and running against the same azure-core-cpp version, right? In that case, I would expect that building+running with azure-core-cpp would work just fine. Or if not, that means our azurefs implementation might not yet be compatible with the latest azure-core-cpp?
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.
Hmm... I tried using 1.11.0 building from source with cmake and everything worked. With 1.10.0 1.11.0 from conda we got segfaults.
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.
You mean 1.11 from conda? (not 1.10) Do you remember if there was more information in the logs about the crash?
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.
#40080 (comment) summarises the info we have.
It looks like 1.11.1 is now available from conda though and that works, so I can remove this version restriction.
@github-actions crossbow submit -g python |
Revision: 53b9d2c Submitted crossbow builds: ursacomputing/crossbow @ actions-47658c1ea8 |
Remaining CI failures: I don't think any of these are related to my changes. |
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 agree CI failures seem unrelated but I'll let @raulcd have the last word on that. Thanks :)
Those failures are unrelated because they are happen on main too. |
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
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit b089c6a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…apache#40080) ### Rationale for this change We want python side tests of `AzureFileSystem` to run in CI. ### What changes are included in this PR? - Add missing `export` to enable Azure pyarrow tests - Enable azure in sdist tests. - Enable Azure on macos python builds - Enable azure in conda builds and install dependencies (Azure C++ SDK and azurite) - Enable retries on C++ tests to mitigate apache#40121 Probably all of this should have been included in apache#39971 ### Are these changes tested? There is no new functionality to test ### Are there any user-facing changes? No * Closes: apache#40079 * GitHub Issue: apache#40079 Authored-by: Thomas Newton <thomas.w.newton@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…apache#40080) ### Rationale for this change We want python side tests of `AzureFileSystem` to run in CI. ### What changes are included in this PR? - Add missing `export` to enable Azure pyarrow tests - Enable azure in sdist tests. - Enable Azure on macos python builds - Enable azure in conda builds and install dependencies (Azure C++ SDK and azurite) - Enable retries on C++ tests to mitigate apache#40121 Probably all of this should have been included in apache#39971 ### Are these changes tested? There is no new functionality to test ### Are there any user-facing changes? No * Closes: apache#40079 * GitHub Issue: apache#40079 Authored-by: Thomas Newton <thomas.w.newton@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
We want python side tests of
AzureFileSystem
to run in CI.What changes are included in this PR?
export
to enable Azure pyarrow testsmain
#40121Probably all of this should have been included in #39971
Are these changes tested?
There is no new functionality to test
Are there any user-facing changes?
No