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

Move cuFile linking to kvikio target #379

Merged
merged 13 commits into from
May 15, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented May 14, 2024

The exported kvikio target has an INTERFACE dependency on cuFile. Add this dependency in the local buildsystem too for consistency, and remove the usage from the examples. This will allow all targets that link against kvikio to automatically get cuFile, including the Python modules.

Fixes: #378

@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner May 14, 2024 18:59
@KyleFromNVIDIA KyleFromNVIDIA added bug Something isn't working non-breaking Introduces a non-breaking change conda labels May 14, 2024
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.

Yes, kvikio's cuFile dependency should be handled the same way we handle cuFile packages in libkvikio. This looks right to me. Thanks!

(I've only ever checked cuFile support for libkvikio because that's what we use in libcudf, so I wasn't aware of this.)

@KyleFromNVIDIA
Copy link
Contributor Author

What I don't understand is why this doesn't already work. libkvikio has a run dependency on cuFile already:

run:
- {{ pin_compatible('cuda-version', max_pin='x', min_pin='x') }}
{% if cuda_major == "11" %}
- cudatoolkit
- libcufile {{ cuda11_libcufile_run_version }} # [linux64]
- libcufile-dev {{ cuda11_libcufile_run_version }} # [linux64]
{% else %}
- libcufile-dev # [linux64]
{% endif %}

@KyleFromNVIDIA
Copy link
Contributor Author

...and it seems this change doesn't work either. I need to go back to the drawing board.

@jakirkham
Copy link
Member

Thanks Kyle! 🙏

Think libkvikio is header only. So adding libcufile-dev doesn't solve things for other libraries, which will need to add libcufile-dev themselves

Am curious if there are more details on what is not working with this change. Would also have expected this to work

@KyleFromNVIDIA
Copy link
Contributor Author

I'm still getting the same error:

Traceback (most recent call last):
  File "/home/kyedwards/development/kvikio/src/python/benchmarks/single-node-io.py", line 429, in <module>
    main(args)
  File "/home/kyedwards/development/kvikio/src/python/benchmarks/single-node-io.py", line 326, in main
    read, write = API[api](args)
                  ^^^^^^^^^^^^^^
  File "/home/kyedwards/development/kvikio/src/python/benchmarks/single-node-io.py", line 42, in run_cufile
    kvikio.memory_register(data)
  File "/home/kyedwards/anaconda3/envs/kvikio-env/lib/python3.11/site-packages/kvikio/__init__.py", line 10, in memory_register
    return libkvikio.memory_register(buf)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "libkvikio.pyx", line 58, in kvikio._lib.libkvikio.memory_register
RuntimeError: KvikIO not compiled with cuFile.h

@jakirkham
Copy link
Member

Could you please share a bit more on how that is coming up? Is that one of the GHA jobs here? Or is that a local build? If so, are there any details?


Taking a look at one of the CUDA 12.2 x86_64 GHA jobs, it appears to be doing the right thing:

 -- Found cuFile: /opt/conda/conda-bld/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/libcufile.so
  -- Found CUDAToolkit: /opt/conda/conda-bld/_build_env/targets/x86_64-linux/include (found version "12.2.140")
  -- Found cuFile Batch API: TRUE
  -- Found cuFile Stream API: TRUE

Same story with one of the CUDA 11.8 x86_64 GHA jobs:

  -- Found cuFile: /opt/conda/conda-bld/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/libcufile.so
  -- Found CUDAToolkit: /usr/local/cuda/targets/x86_64-linux/include (found version "11.8.89")
  -- Found cuFile Batch API: TRUE
  -- Found cuFile Stream API: FALSE

Note that cuFile's Stream API was added in CUDA 12.2. So it is expected to be disabled in CUDA 11.8

Admittedly haven't checked all of the jobs. So could be missing things

@madsbk
Copy link
Member

madsbk commented May 15, 2024

It looks like cmake dosn't run FINAL_CODE_BLOCK when building the Python library.

Reverting #342 fixes the issue.

@madsbk madsbk mentioned this pull request May 15, 2024
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner May 15, 2024 14:33
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner May 15, 2024 14:47
@KyleFromNVIDIA KyleFromNVIDIA changed the title Build Python Conda package with cuFile support Move cuFile linking to kvikio target May 15, 2024
@KyleFromNVIDIA
Copy link
Contributor Author

Local testing seems to indicate that this fix works. Ready for review.

@KyleFromNVIDIA KyleFromNVIDIA requested a review from bdice May 15, 2024 17:26
@KyleFromNVIDIA KyleFromNVIDIA removed the request for review from a team May 15, 2024 19:25
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.

Wonderful! Thank you for the additional comments.

@vyasr
Copy link
Contributor

vyasr commented May 15, 2024

Something seems off here. I agree that the changes are good for FetchContent/add_subdirectory like use cases, but IIUC we were observing issues with conda Python builds which should be finding libkvikio's config, right? Why aren't they?

@KyleFromNVIDIA
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 4355c29 into rapidsai:branch-24.06 May 15, 2024
35 checks passed
@bdice
Copy link
Contributor

bdice commented May 15, 2024

Something seems off here. I agree that the changes are good for FetchContent/add_subdirectory like use cases, but IIUC we were observing issues with conda Python builds which should be finding libkvikio's config, right? Why aren't they?

This was discussed offline and is being fixed in #381.

rapids-bot bot pushed a commit that referenced this pull request May 16, 2024
Use `build.sh` in `meta.yaml`, and pass `-DFIND_KVIKIO_CPP=ON` to CMake.

xref: #379 (comment)

Fixes #378

Authors:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Jake Awe (https://github.com/AyodeAwe)

URL: #381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working conda non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conda package isn't build with GDS support!
6 participants