Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure better compiler cache results between cudf cal-ver branches #11835

Merged
merged 2 commits into from
Oct 25, 2022
Merged

Ensure better compiler cache results between cudf cal-ver branches #11835

merged 2 commits into from
Oct 25, 2022

Conversation

robertmaynard
Copy link
Contributor

Description

By passing the CUDF_VERSION compile definition only to the single source that needs it, we can remove compiler cache misses when switching between branches with different cal-ver values.

Checklist

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

@robertmaynard robertmaynard added bug Something isn't working non-breaking Non-breaking change labels Sep 30, 2022
@robertmaynard robertmaynard requested a review from a team as a code owner September 30, 2022 16:37
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Sep 30, 2022
@robertmaynard
Copy link
Contributor Author

rerun tests

@robertmaynard robertmaynard force-pushed the bug/remove_cudf_version_from_compile_line branch from d267a1e to 4fbdfb4 Compare September 30, 2022 18:22
@codecov
Copy link

codecov bot commented Sep 30, 2022 •
edited
Loading

Codecov Report

Base: 88.11% // Head: 86.88% // Decreases project coverage by -1.22% ⚠️

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

❗ Current head 1fd7ccc differs from pull request most recent head a1b3f56. Consider uploading reports for the commit a1b3f56 to get more accurate results

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-22.12   #11835      +/-   ##
================================================
- Coverage         88.11%   86.88%   -1.23%     
================================================
  Files               133      133              
  Lines             21982    21982              
================================================
- Hits              19369    19100     -269     
- Misses             2613     2882     +269
Impacted Files Coverage Δ
python/cudf/cudf/core/udf/strings_lowering.py 0.00% <0.00%> (-100.00%) ⬇️
python/cudf/cudf/core/udf/strings_typing.py 0.00% <0.00%> (-95.78%) ⬇️
python/strings_udf/strings_udf/lowering.py 0.00% <0.00%> (-84.40%) ⬇️
python/cudf/cudf/core/udf/__init__.py 50.00% <0.00%> (-47.06%) ⬇️
python/strings_udf/strings_udf/_typing.py 81.05% <0.00%> (-13.69%) ⬇️
python/cudf/cudf/core/udf/utils.py 97.00% <0.00%> (-2.00%) ⬇️
python/cudf/cudf/core/column/numerical.py 95.21% <0.00%> (-0.29%) ⬇️
python/strings_udf/strings_udf/__init__.py 84.31% <0.00%> (ø)

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 approved these changes Oct 1, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

One suggestion, otherwise LGTM. Thank you for doing this! I am always happy when cached compile times go down.

cpp/CMakeLists.txt Show resolved Hide resolved
@robertmaynard robertmaynard force-pushed the bug/remove_cudf_version_from_compile_line branch 2 times, most recently from 31c3e80 to b8c02bc Compare October 3, 2022 19:31
@karthikeyann karthikeyann changed the base branch from branch-22.12 to branch-22.10 October 12, 2022 06:42
@karthikeyann karthikeyann changed the base branch from branch-22.10 to branch-22.12 October 12, 2022 06:42
@bdice
Copy link
Contributor

bdice commented Oct 20, 2022

@robertmaynard Is this ready to merge?

@karthikeyann
Copy link
Contributor

There is one more issue that @robertmaynard was investigating - nvbench dependency build repeats everytime in rapids-compose environment.

@robertmaynard
Copy link
Contributor Author

There is one more issue that @robertmaynard was investigating - nvbench dependency build repeats everytime in rapids-compose environment.

That issue is orthongonal to this. It is an issue with how compose, and ccache are setup and shouldn't block this

@robertmaynard
Ensure better compiler cache results between cudf cal-ver branches
33e2cad 
By passing the CUDF_VERSION compile definition only to the single
source that needs it, we can remove compiler cache misses when
switching between branches with different cal-ver values.
@robertmaynard
Correct style issues found by CI
a1b3f56
@robertmaynard robertmaynard force-pushed the bug/remove_cudf_version_from_compile_line branch from b8c02bc to a1b3f56 Compare October 24, 2022 13:49
@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a37f27b into rapidsai:branch-22.12 Oct 25, 2022
@robertmaynard robertmaynard deleted the bug/remove_cudf_version_from_compile_line branch October 25, 2022 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karthikeyann karthikeyann karthikeyann approved these changes

@bdice bdice bdice approved these changes

Assignees
No one assigned
Labels
bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robertmaynard @bdice @karthikeyann