Build KvikIO as a shared library #527

madsbk · 2024-10-30T15:41:49Z

Moving libcurl dependent functions to remote_handle.cpp (let's move the rest in a follow up PR) and setup building libkvikio.so.

Background

Now that KvikIO has evolved into a standalone IO library that works without cuFile and CUDA, I think it makes sense to move to a shared library. Originally, KvikIO was a very thin wrapper around cuFile, which is why we decided to keep it header-only (not because of templates).

Pros:

Enables us to statically compile libcurl into libkvikio.so.
- Required by our wheels. If we don’t do this, we will have to find another solution to Adjust wheel build tags appropriately for new built component #512
Avoid having to compile libcurl in downstream projects. Currently, cudf must compile libcurl in every build.
Reduce compile time in CI, both for KvikIO and downstream projects.
Ease development by not having to rebuild downstream projects like cudf every time KvikIO is modified.

Cons:

Projects cannot vendoring KvikIO as a header-only project. As far as we know, nobody does this.

Revert before PR merge

bdice

This seems mostly right to me. I have a couple pieces of feedback and I noticed CI is not running. It gave an error on the last commit.

Invalid workflow file: .github/workflows/pr.yaml#L78
The workflow is not valid. .github/workflows/pr.yaml (Line: 78, Col: 11): Input shared_actions_repo is required, but not provided while calling. .github/workflows/pr.yaml (Line: 78, Col: 11): Input shared_actions_ref is required, but not provided while calling.

https://github.com/rapidsai/kvikio/actions/runs/11667562812

cpp/include/kvikio/shim/utils.hpp

bdice · 2024-11-04T16:24:27Z

cpp/CMakeLists.txt

@@ -35,14 +35,23 @@ rapids_cmake_write_version_file(include/kvikio/version_config.hpp)
 # Set a default build type if none was specified
 rapids_cmake_build_type(Release)

-# build options
-option(KvikIO_REMOTE_SUPPORT "Configure CMake to build with remote IO support" ON)
+# ##################################################################################################


These comment blocks are super long and the lengths of the --- aren't the same length as the ###. Can we keep this to ~72 chars instead of ~100?

The 100 width of the comment blocks are forced by the cmake style, which adds = until the width is 100. I have fixed the ---- length to match.
I think we need a style update in a separate PR.

bdice · 2024-11-04T16:37:38Z

I noticed CI is not running. It gave an error on the last commit.

This was due to an unexpected merge of rapidsai/shared-workflows#257, which should be corrected by rapidsai/shared-workflows#258. cc: @msarahan

AyodeAwe · 2024-11-04T16:59:20Z

This was due to an unexpected merge of rapidsai/shared-workflows#257, which should be corrected by rapidsai/shared-workflows#258. cc: @msarahan

@bdice perhaps a typo but rapidsai/shared-workflows#257 at this time hasn't been merged.

bdice · 2024-11-04T17:50:17Z

This was due to an unexpected merge of rapidsai/shared-workflows#257, which should be corrected by rapidsai/shared-workflows#258. cc: @msarahan

@bdice perhaps a typo but rapidsai/shared-workflows#257 at this time hasn't been merged.

Sorry, I meant rapidsai/shared-workflows#247. The issue should be resolved now but may require pushing a new commit.

cpp/CMakeLists.txt

Co-authored-by: Robert Maynard <[email protected]>

madsbk · 2024-11-05T18:45:33Z

@robertmaynard, do you have anything else?

vyasr

Looks good. I checked everything, but not exactly with a fine-toothed comb since we'll be testing this very quickly in the downstream PRs anyway.

madsbk · 2024-11-05T20:29:35Z

@bdice do you have anything?

bdice

Approving. I previously looked at everything, and now approve based on the diff since my last look.

madsbk · 2024-11-06T07:00:22Z

/merge

madsbk · 2024-11-06T07:00:32Z

Thanks all. With this merged, we need to update cudf asap: rapidsai/cudf#17239

This reverts commit 2d8eeaf.

Update cudf to use the new KvikIO shared library: rapidsai/kvikio#527 #### Tasks - [x] Wait for the [KvikIO shared library PR](rapidsai/kvikio#527) to be merged. - [x] Revert the use of the [KvikIO shared library](rapidsai/kvikio#527) in CI: 2d8eeaf. Authors: - Mads R. B. Kristensen (https://github.com/madsbk) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - James Lamb (https://github.com/jameslamb) URL: #17239

madsbk added breaking Introduces a breaking change improvement Improves an existing functionality labels Oct 30, 2024

madsbk force-pushed the shared_library branch 9 times, most recently from 31bdb15 to 891099d Compare October 31, 2024 15:43

madsbk added a commit to madsbk/cudf that referenced this pull request Oct 31, 2024

CI: Download wheel from <rapidsai/kvikio#527>

58314a2

madsbk added a commit to madsbk/cudf that referenced this pull request Oct 31, 2024

CI: Download wheel from <rapidsai/kvikio#527>

7cc7611

madsbk added a commit to madsbk/cudf that referenced this pull request Oct 31, 2024

CI: Download wheel from <rapidsai/kvikio#527>

43223ee

madsbk force-pushed the shared_library branch 6 times, most recently from df465f1 to 788c9b4 Compare November 3, 2024 21:19

madsbk added a commit to madsbk/cudf that referenced this pull request Nov 4, 2024

test_wheel: use wheel from <rapidsai/kvikio#527>

20202b0

madsbk force-pushed the shared_library branch 2 times, most recently from 6d38bec to f1a0f21 Compare November 4, 2024 08:29

madsbk added a commit to madsbk/cudf that referenced this pull request Nov 4, 2024

test_wheel: use wheel from <rapidsai/kvikio#527>

abec377

madsbk added a commit to madsbk/cudf that referenced this pull request Nov 4, 2024

more wheel from <rapidsai/kvikio#527>

c929a22

madsbk mentioned this pull request Nov 4, 2024

libcurl: build as shared library #528

Closed

madsbk changed the title ~~Build KvikIO to a shared library~~ Build KvikIO as a shared library Nov 4, 2024

build and use libkvikio.so

6580f1e

madsbk added a commit to madsbk/cudf that referenced this pull request Nov 4, 2024

Download wheel from <rapidsai/kvikio#527>

2d8eeaf

Revert before PR merge

madsbk closed this Nov 4, 2024

doc

c0bf70d

bdice reviewed Nov 4, 2024

View reviewed changes

madsbk added 2 commits November 4, 2024 20:11

typo

7488b9a

fix style

73a8eb9

robertmaynard requested changes Nov 4, 2024

View reviewed changes

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

Use LINKER instread of -wl,

a267d30

Co-authored-by: Robert Maynard <[email protected]>

madsbk requested a review from robertmaynard November 4, 2024 20:04

madsbk and others added 3 commits November 5, 2024 09:05

Merge branch 'branch-24.12' into shared_library

b0bc998

remove deprecated VAULT_HOST and use AWS_ROLE_ARN instead

8421287

generate build-dir package

a732487

robertmaynard approved these changes Nov 5, 2024

View reviewed changes

vyasr approved these changes Nov 5, 2024

View reviewed changes

bdice approved these changes Nov 5, 2024

View reviewed changes

rapids-bot bot merged commit 1c99841 into rapidsai:branch-24.12 Nov 6, 2024
57 checks passed

madsbk added a commit to madsbk/cudf that referenced this pull request Nov 6, 2024

Revert "Download wheel from <rapidsai/kvikio#527>"

c0461fe

This reverts commit 2d8eeaf.

pxLi mentioned this pull request Nov 6, 2024

[BUG] submodule syncup failed build kvikIO related code NVIDIA/spark-rapids-jni#2576

Closed

jlowe mentioned this pull request Nov 6, 2024

kvikio fails to build with -DKvikIO_REMOTE_SUPPORT=OFF #538

Closed

This was referenced Nov 6, 2024

Add support for cufile to rapids-cpm rapidsai/rapids-cmake#717

Open

Audit CMake for manually handling properties that should be handled by targets or rapids-cmake functions #541

Open

Adjust wheel build tags appropriately for new built component #512

Closed

This was referenced Nov 8, 2024

[BUG] nightly build failed kvikIO in cuda11+arm64 NVIDIA/spark-rapids-jni#2580

Closed

[BUG] Linking fails due to missing kvikIO symbols NVIDIA/spark-rapids-jni#2581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build KvikIO as a shared library #527

Build KvikIO as a shared library #527

madsbk commented Oct 30, 2024 •

edited

Loading

bdice left a comment •

edited

Loading

bdice Nov 4, 2024

madsbk Nov 4, 2024

bdice commented Nov 4, 2024

AyodeAwe commented Nov 4, 2024

bdice commented Nov 4, 2024 •

edited

Loading

madsbk commented Nov 5, 2024

vyasr left a comment

madsbk commented Nov 5, 2024

bdice left a comment

madsbk commented Nov 6, 2024

madsbk commented Nov 6, 2024

Build KvikIO as a shared library #527

Build KvikIO as a shared library #527

Conversation

madsbk commented Oct 30, 2024 • edited Loading

Background

bdice left a comment • edited Loading

Choose a reason for hiding this comment

bdice Nov 4, 2024

Choose a reason for hiding this comment

madsbk Nov 4, 2024

Choose a reason for hiding this comment

bdice commented Nov 4, 2024

AyodeAwe commented Nov 4, 2024

bdice commented Nov 4, 2024 • edited Loading

madsbk commented Nov 5, 2024

vyasr left a comment

Choose a reason for hiding this comment

madsbk commented Nov 5, 2024

bdice left a comment

Choose a reason for hiding this comment

madsbk commented Nov 6, 2024

madsbk commented Nov 6, 2024

madsbk commented Oct 30, 2024 •

edited

Loading

bdice left a comment •

edited

Loading

bdice commented Nov 4, 2024 •

edited

Loading