Skip to content

Fix include line for IO Cython modules - #12250

Merged
AyodeAwe merged 1 commit into
NVIDIA:branch-22.12from
vyasr:fix/io_numpy_link
Nov 29, 2022
Merged

Fix include line for IO Cython modules#12250
AyodeAwe merged 1 commit into
NVIDIA:branch-22.12from
vyasr:fix/io_numpy_link

Conversation

@vyasr

@vyasr vyasr commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

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 charlesbluca and wence- 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

bdice commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

How does this relate to the comment below? Does something else need changed/updated? https://github.com/rapidsai/cudf/blob/2a5411ad4624c771747e2cd45e368156b1f91fbe/python/cudf/cudf/_lib/CMakeLists.txt#L82-L84

@vyasr

vyasr commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

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

https://github.com/rapidsai/cudf/blob/2a5411ad4624c771747e2cd45e368156b1f91fbe/python/cudf/cudf/_lib/CMakeLists.txt#L82-L84

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

codecov Bot commented Nov 29, 2022

Copy link
Copy Markdown

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 NVIDIA: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