numba / llvmlite

A lightweight LLVM python binding for writing JIT compilers
https://llvmlite.pydata.org/
BSD 2-Clause "Simplified" License
1.94k stars 322 forks source link

Create GitHub Action for llvmlite release #904

Closed apmasell closed 1 year ago

apmasell commented 1 year ago

Build a new GitHub Action to build wheels for llvmlite

esc commented 1 year ago

I have completed a visual inspection of the proposed changes, and they look fine to me. The comments are a bit sparse,

But, the big question is how do we test this? @apmasell how did you test this when you developed it? For PyPi, I think there is a pypi test server where we can upload stuff? For the GitHub releases, do we run this in a fork and then check the artifacts?

Potential additions:

I appreciate the use of the ™️ character.

apmasell commented 1 year ago

You can see the artifacts that are built on my fork: https://github.com/apmasell/llvmlite/releases/tag/v99.99.99 As for testing the PyPI part, I haven't found a good way to do that. I think that's a non-critical failure. If it fails during the release, we can manually upload the wheels it builds as we do currently and adjust the action

How do we currently verify the llvmdev version?

esc commented 1 year ago

You can see the artifacts that are built on my fork: https://github.com/apmasell/llvmlite/releases/tag/v99.99.99 As for testing the PyPI part, I haven't found a good way to do that. I think that's a non-critical failure. If it fails during the release, we can manually upload the wheels it builds as we do currently and adjust the action

How do we currently verify the llvmdev version?

We install using numba::llvmdev=14 ...

esc commented 1 year ago

I think it'll be fine like this, actually, when would be a good time to merge this? after the current release?

apmasell commented 1 year ago

I'm confused. Should numba::llvmdev=14 be different than the environment file?

This should get merged after the release, I think.

esc commented 1 year ago

I'm confused. Should numba::llvmdev=14 be different than the environment file?

I don't know exactly, but the following two have sometimes produced different results in the past:

conda install -c numba llvmdev`

and

conda install numba::llvmdev

Hence I am advocating for caution here, that we don't run into any conda bugs.

This should get merged after the release, I think.

I concur.

apmasell commented 1 year ago

Okay. Unfortunately, the environment files don't allow the channel::package syntax.

esc commented 1 year ago

Okay. Unfortunately, the environment files don't allow the channel::package syntax.

I see, in that case, maybe we do want to check if the correct llvmdev was installed. Maybe a small bash one-liner?

apmasell commented 1 year ago

Sigh. This is the wrong fork. I've pushed a different method.

esc commented 1 year ago

@apmasell what do I look at next to continue the review?

apmasell commented 1 year ago

I've replaced the total work, so please pretend it's a fresh PR.

esc commented 1 year ago

@apmasell thank you for the updates, I have given this a first pass to familiarize myself with the code and found a couple of typos in the process. I mostly understand the setup, as I have been doing llvmlite releases in the past.

Since this now produces artifacts, we need to devise a strategy to test them accordingly. Ideally, I would like to test each python x platform combination of wheel in combination with an appropriate Numba. The real litmus test for llvmlite is always the Numba test suit. Do you have any ideas as to how we might go about this?

apmasell commented 1 year ago

I've made the changes requested. For testing, I'm not sure. We have a manual validation process for the existing packages, so I think the right reasonable thing to do is merge this as-is, allow it to build packages during the next, compare those packages to the packages we've build using existing tooling and the same validation. We can then make a better informed decision about what validation we need.

It is my understand that the release process checks that the build tag is good and then assumes that the package built from that tag needs minimal validation. I think that's something we might want to re-evaluate. Doing anything heavy-weight will require us to get more GitHub Action compute resources, so, that will have to wait until later.

esc commented 1 year ago

I've made the changes requested. For testing, I'm not sure. We have a manual validation process for the existing packages, so I think the right reasonable thing to do is merge this as-is, allow it to build packages during the next, compare those packages to the packages we've build using existing tooling and the same validation. We can then make a better informed decision about what validation we need.

OK

It is my understand that the release process checks that the build tag is good and then assumes that the package built from that tag needs minimal validation. I think that's something we might want to re-evaluate. Doing anything heavy-weight will require us to get more GitHub Action compute resources, so, that will have to wait until later.

Understood.

Fire in the hole then 🔥 🕳️