Add CI jobs #10

brandon-b-miller · 2023-10-31T14:34:13Z

This PR adds a CI workflow that runs on each PR.

pentschev · 2023-10-31T14:43:18Z

I think this has to be under .github/workflows/pr.yaml. Additionally the way CI is currently setup probably means you need other steps, for example a .github/workflows/build.yaml. Is the intent to make pynvjitlink a full conda-forge/wheels package?

brandon-b-miller · 2023-10-31T14:45:48Z

I think this has to be under .github/workflows/pr.yaml. Additionally the way CI is currently setup probably means you need other steps, for example a .github/workflows/build.yaml. Is the intent to make pynvjitlink a full conda-forge/wheels package?

Yes, we should ultimately have both wheels and conda packages.

pentschev · 2023-10-31T14:57:13Z

Ok, then we probably need to do the entire setup here. That means we will need to copy https://github.com/rapidsai/cudf/blob/branch-23.12/.github/copy-pr-bot.yaml and https://github.com/rapidsai/cudf/blob/branch-23.12/.github/ops-bot.yaml into this PR. We will then need to add conda recipes and wheels build scripts. I can probably help get started here but we will also need help from @bdice and @vyasr who are the experts in packaging.

For the conda recipe, the closest recipe to pynvjitlink I know of is perhaps UCX-Py, so what I would do is first copy its conda files and remove UCX dependencies, probably replace UCX with Numba. We will then need to add build and tests CI scripts, which I would do the same, copy from UCX-Py and replace relevant ucx-py with pynvjitlink. Wheels build and tests scripts are also in the same ci/ subdirectory and probably can get us a starting point, there are no recipes like in conda but we often require static linking, where @vyasr is definitely someone who we will need help from.

If you want, please ping me on Slack and I can try helping get moving here a bit more, but most definitely we will need more help from Bradley and Vyas later anyway.

brandon-b-miller · 2023-10-31T15:03:47Z

@pentschev we've met once or twice and made some progress planning this out - will loop you in on slack.

.github/workflows/pr.yaml

bdice · 2023-11-01T21:58:50Z

ci/build_wheel.sh

+
+rapids-logger "Installing wheel"
+
+pip install pynvjitlink-0.1.0-cp310-cp310-manylinux_2_35_x86_64.whl


We may need to use an older CI image than what is pinned at rapidsai/ci-conda:latest because we want to use the oldest OS to ensure that wheels are compatible with an older manylinux tag. This should be manylinux_2_17 if at all possible. Try using a CentOS 7 image and see if that helps.

I just noticed that we're going to need a Python version matrix. This wheel only support Python 3.10. We need to ship pynvjitlink on Python 3.9, 3.10 for RAPIDS, and also 3.11 and 3.12 if we want to make external consumers from Numba happy. We don't have Python 3.11 / 3.12 Docker images yet.

@gmarkall Can you comment on the expected Python version support for this package?

I was hoping to follow SPEC 0, so at the present time 3.10-3.12 support would be my aim. If 3.9 support is also needed for RAPIDS then that seems OK. If we can't initially provide wheels for 3.11 and 3.12 then I think that's also OK.

My reasoning is that for non-RAPIDS use cases, it will always be possible for users to build pynvjitlink themselves (unlike with cubinlinker), and we're covering the RAPIDS deployment scenarios in supporting 3.9 and 3.10 from the start. If there's no short-term route to subsequently adding 3.11 and 3.12, I'd like to look into how to get to supporting them eventually.

It looks like there aren't ci-conda centos images spanning the range of python versions we need. Should we be installing the right version of python ourselves within the container or is there something else I should be doing instead?

You can add newer Python CI images, or just use Python versions from conda. Perhaps it will suffice to have a single build job that iterates over all the Python versions, since the builds should be fast.

I think I'd lean towards having a separate job for each build, just so that if something goes wrong with just one of them one doesn't have to parse through the log to figure out which one.

Installing and using the correct python version via conda seems like it's a fairly easy, so I could investigate down that route. What do you mean by "add newer python CI images"?

What do you mean by "add newer python CI images"?

Today, our CI images only cover Python 3.9 and 3.10. https://github.com/rapidsai/ci-imgs/blob/bf7d540855e7e94695a54e40080c3acb8681764a/matrix.yaml#L8-L10

Perhaps it will suffice to have a single build job that iterates over all the Python versions, since the builds should be fast.

I had in mind using a conda variant build like in ucxx, where the conda_build_config.yaml has a list of Python versions and conda build knows to build all of them. It can be quite convenient, especially since pynvjitlink may have a wider support matrix of Python versions than RAPIDS as a whole (and is already likely to be using a newer CUDA than we might be using in RAPIDS).

https://github.com/rapidsai/ucxx/blob/4464ca942d5ac71aabe4492e2a6d59da453f06dc/conda/recipes/ucxx/conda_build_config.yaml#L19-L21

ci/build_wheel.sh

ci/check_style.sh

.github/workflows/pr.yaml

Co-authored-by: Bradley Dice <[email protected]>

.github/workflows/pr.yaml

brandon-b-miller · 2023-11-15T23:00:47Z

pynvjitlink/tests/test_pynvjitlink_api.py

@@ -98,13 +104,15 @@ def test_add_cubin_with_fatbin_error(device_functions_fatbin):
        nvjitlinker.add_cubin(device_functions_fatbin, name)


+@pytest.mark.skip


TODO: not sure why we need to skip this in CI but it works for me locally.

Maybe this should be xfailed in CI instead of skipped?

I think we still want to skip things due to #3

.github/workflows/pr.yaml

bdice · 2023-11-15T23:05:32Z

.github/workflows/pr.yaml

+        python-version: ["3.9", "3.10"]
+        linux-version: ["centos7", "rockylinux8"]
+    container:
+      image: "rapidsai/citestwheel:cuda12.0.1-ubuntu20.04-py${{ matrix.python-version }}"


Is linux-version: ["centos7", "rockylinux8"] being used? I would've expected it here.

This has been updated. To be honest I'm not 100% sure what should be here. I noticed that in cuDF we only test wheels on ubuntu, albeit testing on two versions, and only testing arm with ubuntu 20.04. This seems to line up with what's available for docker under rapidsai/citestwheel, so I switched it to mirror that here, let me know if that seems to line up with what we want.

bdice · 2023-11-15T23:06:03Z

.github/workflows/pr.yaml

+    strategy:
+      matrix:
+        python-version: ["3.9", "3.10"]
+        linux-version: ["centos7", "rockylinux8"]


Same question as above about linux-version being unused.

CMakeLists.txt

ci/build_wheel.sh

ci/test_wheel.sh

pynvjitlink/tests/test_pynvjitlink.py

bdice · 2023-11-15T23:13:49Z

pynvjitlink/tests/test_pynvjitlink_api.py

@@ -98,13 +104,15 @@ def test_add_cubin_with_fatbin_error(device_functions_fatbin):
        nvjitlinker.add_cubin(device_functions_fatbin, name)


+@pytest.mark.skip


Maybe this should be xfailed in CI instead of skipped?

Co-authored-by: Bradley Dice <[email protected]>

This reverts commit 78b0b41.

quasiben · 2023-11-16T19:41:08Z

@gmarkall and I met to fix up tests. I'll follow up in a separate PR to fix those tests. For now it seems like PR is ready for review then merge

bdice

This looks good to me. I think we can merge this and test the artifacts, if you agree?

bdice · 2023-11-17T04:05:45Z

ci/build_wheel.sh

+conda list
+
+rapids-logger "Build wheel"
+export SCCACHE_S3_NO_CREDENTIALS=1


I think the follow-up task here is to try using the aws-actions/configure-aws-credentials@v3 step that I linked above. It doesn't matter for this PR, though.

bdice · 2023-11-17T04:09:31Z

This adds CI to PRs -- but we will need a build.yaml to make this publish the wheels to a nightly index for testing (maybe triggered on tags?). @brandon-b-miller and @quasiben, are we ready for that? I think so?

gmarkall

I think this is good to merge - there are some follow-ups that I think we need like test amendments (and figuring out why the library is linked against libcuda.so), but it would be good to get this in then address those in follow-up PRs.

brandon-b-miller · 2023-11-17T14:08:21Z

This adds CI to PRs -- but we will need a build.yaml to make this publish the wheels to a nightly index for testing (maybe triggered on tags?). @brandon-b-miller and @quasiben, are we ready for that? I think so?

This seems like a logical next step to me

add initial check

76d4540

move pr.yaml

71f31dc

brandon-b-miller added 16 commits October 31, 2023 08:08

add empty check_style.sh

cadc4bc

basic check_style.sh that does nothing

3f46b26

add +x

2dbd68f

try adding a wheel-build job

be8c202

fix spacing

695a133

add runs-on

cea436a

install ctk

bc7116f

add -q for now

351fd7e

switch to apt-get

b18b582

minor fixes

c3ebe4c

update package name

cb21a8e

add install options

7e67976

expanding build_wheel.sh

487341d

add tests and logging

e58540a

more test build steps

d40ce2e

fix path

cefb8f6

brandon-b-miller changed the title ~~[TEST] Try adding a CI check~~ Add CI jobs Nov 1, 2023

bdice reviewed Nov 1, 2023

View reviewed changes

.github/workflows/pr.yaml Outdated Show resolved Hide resolved

brandon-b-miller and others added 2 commits November 2, 2023 09:33

Apply suggestions from code review

29b319d

Co-authored-by: Bradley Dice <[email protected]>

fix style

b4ef9f6

brandon-b-miller requested a review from a team as a code owner November 2, 2023 14:45

brandon-b-miller added 2 commits November 2, 2023 07:47

add wheel-build to needs

a3fef15

apt-get -> yum due to switching to centos

5a36b01

brandon-b-miller added 6 commits November 15, 2023 11:30

do not set e for now

ee7841d

use SCCACHE_S3_NO_CREDENTIALS=1

774b2c8

use ci-wheel containers to get arm images

7449fcb

get python from the image instead of conda

e11cc48

use inputs.node_type

7ef7ce7

build on cpu runners

ce3bb98

brandon-b-miller commented Nov 15, 2023

View reviewed changes

.github/workflows/pr.yaml Show resolved Hide resolved

brandon-b-miller and others added 6 commits November 15, 2023 15:27

Update .github/workflows/pr.yaml

6497681

remove python version from path

f340a61

separate testing into arm64 and amd64

4614362

add to needs section

6baf691

use citestwheel test images

b4db71a

skip bad test

ca82b03

brandon-b-miller commented Nov 15, 2023

View reviewed changes

bdice reviewed Nov 15, 2023

View reviewed changes

bdice and others added 8 commits November 15, 2023 17:14

Apply suggestions from code review

5ea590e

Update pynvjitlink/tests/test_pynvjitlink.py

110af93

Co-authored-by: Bradley Dice <[email protected]>

build amd and arm in two separate jobs

c9f57b4

don't test ubuntu 18.04 with arm

0789176

address reviews

f016805

fix cmake

1f647ba

try removing cache again

78b0b41

Revert "try removing cache again"

7e48e2b

This reverts commit 78b0b41.

bdice approved these changes Nov 17, 2023

View reviewed changes

gmarkall approved these changes Nov 17, 2023

View reviewed changes

brandon-b-miller merged commit c540358 into rapidsai:main Nov 17, 2023
13 checks passed

gmarkall mentioned this pull request Nov 20, 2023

Add support for LTO in Numba linker #11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CI jobs #10

Add CI jobs #10

brandon-b-miller commented Oct 31, 2023

pentschev commented Oct 31, 2023

brandon-b-miller commented Oct 31, 2023

pentschev commented Oct 31, 2023

brandon-b-miller commented Oct 31, 2023

bdice Nov 1, 2023

bdice Nov 1, 2023

bdice Nov 1, 2023

gmarkall Nov 3, 2023

brandon-b-miller Nov 3, 2023

bdice Nov 3, 2023

brandon-b-miller Nov 6, 2023

bdice Nov 7, 2023

bdice Nov 7, 2023 •

edited

Loading

brandon-b-miller Nov 15, 2023

bdice Nov 15, 2023

brandon-b-miller Nov 16, 2023

bdice Nov 15, 2023

brandon-b-miller Nov 16, 2023

bdice Nov 15, 2023

bdice Nov 15, 2023

quasiben commented Nov 16, 2023

bdice left a comment

bdice Nov 17, 2023

bdice commented Nov 17, 2023 •

edited

Loading

gmarkall left a comment

brandon-b-miller commented Nov 17, 2023


		rapids-logger "Installing wheel"

		pip install pynvjitlink-0.1.0-cp310-cp310-manylinux_2_35_x86_64.whl

		@@ -98,13 +104,15 @@ def test_add_cubin_with_fatbin_error(device_functions_fatbin):
		nvjitlinker.add_cubin(device_functions_fatbin, name)


		@pytest.mark.skip

Add CI jobs #10

Add CI jobs #10

Conversation

brandon-b-miller commented Oct 31, 2023

pentschev commented Oct 31, 2023

brandon-b-miller commented Oct 31, 2023

pentschev commented Oct 31, 2023

brandon-b-miller commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quasiben commented Nov 16, 2023

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice commented Nov 17, 2023 • edited Loading

gmarkall left a comment

Choose a reason for hiding this comment

brandon-b-miller commented Nov 17, 2023

bdice Nov 7, 2023 •

edited

Loading

bdice commented Nov 17, 2023 •

edited

Loading