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.

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

Fix include line for IO Cython modules #12250

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 29, 2022

Description

This PR ensures that numpy include directories are visible to the cuIO Cython TUs. #12096 changed the way that numpy was being found but did not update this line. The old variable is automatically being created by scikit-build's Python code, so as long as skbuild correctly finds the libraries the old variable still works, but we do not want to rely on that since it is not true in all environments.

Checklist

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

@vyasr vyasr added bug Something isn't working 3 - Ready for Review Ready for review by team ! - Hotfix Hotfix is a bug that affects the majority of users for which there is no reasonable workaround non-breaking Non-breaking change labels Nov 29, 2022
@vyasr vyasr requested a review from a team as a code owner November 29, 2022 00:44
@vyasr vyasr self-assigned this Nov 29, 2022
@vyasr vyasr requested review from wence- and charlesbluca and removed request for a team November 29, 2022 00:44
@github-actions github-actions bot added CMake CMake build issue Python Affects Python cuDF API. labels Nov 29, 2022
@bdice
Copy link
Contributor

bdice commented Nov 29, 2022

How does this relate to the comment below? Does something else need changed/updated?

target_include_directories(${target} PRIVATE "${NumPy_INCLUDE_DIRS}")
# Switch to the line below when we switch back to FindPython.cmake in CMake 3.24.
# target_include_directories(${target} PRIVATE "${Python_NumPy_INCLUDE_DIRS}")

@vyasr
Copy link
Contributor Author

vyasr commented Nov 29, 2022

How does this relate to the comment below? Does something else need changed/updated?

target_include_directories(${target} PRIVATE "${NumPy_INCLUDE_DIRS}")
# Switch to the line below when we switch back to FindPython.cmake in CMake 3.24.
# target_include_directories(${target} PRIVATE "${Python_NumPy_INCLUDE_DIRS}")

That comment is referring to the changes I made to _lib/CMakeLists.txt in the wheels PR. This PR is updating _lib/io/CMakeLists.txt, which was relying on the variables that were previously populated by the old find_package call in _lib/CMakeLists.txt. I changed one call without changing the other.

@vyasr vyasr removed the ! - Hotfix Hotfix is a bug that affects the majority of users for which there is no reasonable workaround label Nov 29, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Base: 88.25% // Head: 88.25% // No change to project coverage 👍

Coverage data is based on head (c83ff55) compared to base (0c60819).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-22.12   #12250   +/-   ##
=============================================
  Coverage         88.25%   88.25%           
=============================================
  Files               137      137           
  Lines             22571    22571           
=============================================
  Hits              19921    19921           
  Misses             2650     2650           

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.

@AyodeAwe AyodeAwe merged commit eb27104 into rapidsai:branch-22.12 Nov 29, 2022
@vyasr vyasr deleted the fix/io_numpy_link branch November 29, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working CMake CMake build issue non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants