Skip to content

Determine if Arrow has S3 support at runtime in unit test. - #11560

Merged
rapids-bot[bot] merged 2 commits into
NVIDIA:branch-22.12from
bdice:fix-arrow-s3-test
Oct 26, 2022
Merged

Determine if Arrow has S3 support at runtime in unit test.#11560
rapids-bot[bot] merged 2 commits into
NVIDIA:branch-22.12from
bdice:fix-arrow-s3-test

Conversation

@bdice

@bdice bdice commented Aug 17, 2022

Copy link
Copy Markdown
Contributor

Description

Resolves #11559. This PR improves the logic for testing S3 support. Previously this test relied on the value of CUDF_ENABLE_ARROW_S3, which only enables S3 support in Arrow if Arrow is being built from source by libcudf. If the Arrow package is found locally (rather than fetched and built), the value of CUDF_ENABLE_ARROW_S3 was irrelevant. Therefore, the tests using the compile-time value of CUDF_ENABLE_ARROW_S3 were unable to correctly detect Arrow's S3 support. This PR fixes the problem by checking Arrow S3 support at runtime.

I tested this locally for the case where Arrow doesn't have S3 support (our CI uses prebuilt Arrow packages with S3 enabled).

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 17, 2022
@codecov

codecov Bot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Base: 88.11% // Head: 88.14% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (ce10aae) compared to base (5c2150e).
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.12   #11560      +/-   ##
================================================
+ Coverage         88.11%   88.14%   +0.03%     
================================================
  Files               133      133              
  Lines             21982    21982              
================================================
+ Hits              19369    19376       +7     
+ Misses             2613     2606       -7     
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 93.77% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.65% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.51% <0.00%> (+0.20%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 93.75% <0.00%> (+0.96%) ⬆️
python/strings_udf/strings_udf/__init__.py 86.27% <0.00%> (+1.96%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bdice bdice self-assigned this Aug 18, 2022
@bdice bdice added bug Something isn't working tests Unit testing for project non-breaking Non-breaking change labels Aug 18, 2022
@github-actions

Copy link
Copy Markdown

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@bdice
bdice changed the base branch from branch-22.10 to branch-22.12 October 20, 2022 19:46
@bdice
bdice marked this pull request as ready for review October 24, 2022 15:16
@bdice
bdice requested a review from a team as a code owner October 24, 2022 15:16
@bdice
bdice requested review from mythrocks and nvdbaranec October 24, 2022 15:16

@upsj upsj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Does the CI setup test both configurations? Otherwise we might miss the failure detection breaking silently

@bdice

bdice commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

LGTM! Does the CI setup test both configurations? Otherwise we might miss the failure detection breaking silently

No, we don't have a way to test the configuration with S3 disabled in CI. We would have to make significant changes to how dependencies are installed in CI. It might be possible once CI is more configurable via a GitHub Actions matrix build, but I don't think the benefit is worth the cost here.

@bdice

bdice commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 72572a8 into NVIDIA:branch-22.12 Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Arrow S3 tests don't throw an error when CUDF_ENABLE_ARROW_S3 is off

3 participants