Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add CI jobs #10
Add CI jobs #10
Changes from 18 commits
76d4540
71f31dc
cadc4bc
3f46b26
2dbd68f
be8c202
695a133
cea436a
bc7116f
351fd7e
b18b582
c3ebe4c
cb21a8e
7e67976
487341d
e58540a
d40ce2e
cefb8f6
29b319d
b4ef9f6
a3fef15
5a36b01
37ce733
cca7db5
abeae67
8af9c15
510db4a
1335b56
cb57054
25bd2b7
1669c92
42b0c5b
40b8d91
f84ba0b
946509c
a6e76c7
f59a41a
306c1b9
7ea3af1
1b2ce4d
02906bb
91ac502
08842b1
1dc9460
a70ea01
a635037
8dcceef
5690d0a
667691f
e253487
b0b0410
e0f71aa
ab57c23
e2a2a3b
8c318ff
8dfd9b9
d1eac13
78566e0
effab51
b80ad0a
ee7841d
774b2c8
7449fcb
e11cc48
7ef7ce7
ce3bb98
6497681
f340a61
4614362
6baf691
b4db71a
ca82b03
5ea590e
110af93
c9f57b4
0789176
f016805
1f647ba
78b0b41
7e48e2b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bemanylinux_2_17
if at all possible. Try using a CentOS 7 image and see if that helps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmarkall Can you comment on the expected Python version support for this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, our CI images only cover Python 3.9 and 3.10. https://github.com/rapidsai/ci-imgs/blob/bf7d540855e7e94695a54e40080c3acb8681764a/matrix.yaml#L8-L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had in mind using a conda variant build like in ucxx, where the
conda_build_config.yaml
has a list of Python versions andconda 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