inducer / pyopencl

OpenCL integration for Python, plus shiny features
http://mathema.tician.de/software/pyopencl
Other
1.05k stars 241 forks source link

ci+cd.feat(wheel & pypi): improve and simplify the process and support more platforms #489

Closed yxliang01 closed 2 years ago

yxliang01 commented 3 years ago

This changes the GitHub action to use cibuildwheel tool to build the wheels, was using manylinux docker instead. By moving to the more generic cibuildwheel, the concrete platform-specific steps to build wheels are encapsulated. This introduction allows building wheels for more os a lot easier (support is TODO). As a side effect, pypy wheels are now built.

Second part is to use the GitHub action for publishing to pypi from pypa with simplification to the checking logic. This action is also recommended for usage in the GitHub official docs for publishing to pypi.

This closes #488 closes #364 . This is a step for #420 and #406 .

P.S. I finally made this PR after planning for a year :joy: :tada:

yxliang01 commented 3 years ago

Third, publish to pypi only when GitHub release is created. This promotes a better publish workflow I believe. Fourth, simplified multiple build scripts.

TODO: support wheel testing in future PR

@inducer I'm happy to suggest that this PR is ready to review! The publish function will still need to be tested though. Note that this PR introduces new third-party dependencies.

isuruf commented 3 years ago

Thanks @yxliang01 for the PR. I like some of the new features (like pypy support), but I'm not sure we'd want the indirection of cibw which makes this hard to debug. Things like adding pypy support can be added without cibw right?

yxliang01 commented 3 years ago

cibw is not intended to be a replacement of anything or even be something required. But, it encapsulates the building processes and adapt automatically for different python implementation and platforms and archs, this is why I propose to use it instead of almost completely reinventing the wheels. And, it doesn't do anything seems too magical. In fact, if you look at the output produced, it does a great job at logging and since many things are automatically done, I expect the need for debugging the wheel building could then be less. So, I'm not sure how cibw can make debugging harder, but happy to have some discussions anyways.

isuruf commented 3 years ago

But, it encapsulates the building processes and adapt automatically for different python implementation and platforms and archs

I expect the building processes to vary wildly for macos and windows. As you can see from the Linux build, it's certainly non standard and required changing cibw defaults to the point where we lose all the goodies that cibw brings.

propose to use it instead of almost completely reinventing the wheels

I agree that it's not good to reinvent the wheel. (pun intended), I look at scripts/prepare-build-wheels.sh vs scripts/build-wheels.sh and there's hardly any difference and the only thing different is the loop of python versions.

So, I'm not sure how cibw can make debugging harder, but happy to have some discussions anyways.

What I mean is that, when debugging these builds, what I do is I start the same docker container and run the build script which is easy enough. With the indirection of cibw, I'm not sure how to debug these builds.

yxliang01 commented 3 years ago

So the main point of introducing cibw is the encapsulation it provides which can bring the following benefits (1) allows us organize different components in wheel building (2) not maintaining some things specific to the environment (e.g. arch, platform, implementation) (3) be more future-proof because when any of the workflow changes, cibw should be changed accordingly by pypa team.

It's true that in this PR, quite some customization is done. But, most of them are actually not required, e.g. specifying python version. They are here because some of the things weren't done in a standard way, e.g. declaring the build-time dependency. I didn't try to simplify it in this PR since I don't want to change too many less related things at one time.

And, @isuruf pointed out the increased difficulty of debugging. I do agree with this, I guess it can be a commonly seemed potential drawback of increased encapsulation. In this case, I think it shouldn't make debugging a lot more difficult because the logging is very good that we can know most of the things cibw did for us and we can also run cibw ourselves to debug, just similar to you manually run docker command but simpler. Yet another potential drawback is the inflexibility of customization because of the semi-fixed workflow, e.g. can't provide additional docker arguments to docker run. But, so far, I think we have the flexibility needed and don't expect it won't meet our need in near future.

So, cibw brings in a way for us to decouple our wheel building process from manylinux2014 which is something wanted in order to provide wheels for macos and windows. Decoupling doesn't require cibw but it's an easily accessible solution and well-maintained by pypa, so why not? One more thing we can quickly do with cibw is the wheel testing. It would require us writing some more logics before it can be done properly while with cibw, just setting the right environment variable should be sufficient.

P.S. our team is using pyopencl on pypy3. The main motivation of me porting the building to upstream is also that we don't want the pyopencl building logic in our repos and we need to maintain separately :joy:

isuruf commented 3 years ago

First, thanks for describing your though process in detail. It helps to understand what you are trying to achieve.

(1) allows us organize different components in wheel building

I don't understand what this means

(2) not maintaining some things specific to the environment (e.g. arch, platform, implementation)

Both scripts/prepare-build-wheels.sh from this PR and scripts/build-wheels.sh works for all Linux builds (arch, implementation) and is useless for other OSes. ` So, I don't see any difference with the current script and cibw.

(3) be more future-proof because when any of the workflow changes, cibw should be changed accordingly by pypa team.

I don't see how this would change.

So, cibw brings in a way for us to decouple our wheel building process from manylinux2014 which is something wanted in order to provide wheels for macos and windows.

This doesn't make sense to me. For macos and windows the wheel building process is going to be extremely different than the Linux one due to the libOpenCL dependency and how they interact with the system implementations.

P.S. our team is using pyopencl on pypy3. The main motivation of me porting the building to upstream is also that we don't want the pyopencl building logic in our repos and we need to maintain separately joy

Cool. I sent https://github.com/inducer/pyopencl/pull/491 for this.

isuruf commented 3 years ago

Btw, the debugging issue is not about debugging building the wheel, but it's mostly about ocl-icd and the patches we carry to make it work with binary distributions like pocl-binary-distribution.

inducer commented 3 years ago

@yxliang01 Thanks for your work here, and thanks @isuruf for chiming in! I'm a bit torn TBH. On the one hand, I like the potential simplicity of cibw, on the other hand I do share @isuruf's concerns about system-specific aspects of OpenCL that could very well negate the benefits. Given that the immediate objective appears to be pypy support, I suspect #491 is the less disruptive change. Nonetheless, let's leave this open to consider in case the "manual" build setup requires substantial maintenance in the future.

yxliang01 commented 3 years ago

Thanks @isuruf for #491 !

So to answer your previous response: For point 1, what I meant is that it gives a way to decompose the wheel building process into multiple components, e.g. prepare env, how to repair wheels, etc... Also, it gives us a way to separate parts specific each platform, e.g. call different prepare scripts for different os.

For point 2, I was only focusing on the wheel building and repairing and testing part only because the environment preparation like libOpenCL ocl-icd building is not related to cibw at all.

For point 3, I don't know either since it isn't happening. But, with cibw, we don't need to care it at all. e.g. we don't care how the pypi API endpoint changes because we are using twine. The same for cibw.

I believe there was a bit misunderstanding that I focused on the wheel building, repairing and testing while @isuruf is on the environment preparation. Since cibw is clearly not dealing anything related to our own customized preparation, so I don't think it affects much the debugging of it.

I'm not sure how to build successfully wheels in Windows and macos, so actually can't comment much. But anyways, my main goal was to have pypy wheels provided by you guys. I'm aware that the support can be added nowadays by simply changing the bash script a little bit like #491. But then, I realized that the building script has a bit too much complexity than it needs to be and saw many others also ask for wheels for other environments. That's why I was thinking to do a little bit more to make things better and moving and created #488 to ask whether cibw is wanted. Followed by @inducer 's positive comment, I started on this with another hope that support for other platforms can be added sooner than without this PR because it's conceptually easier to be done than without cibw. e.g. with cibw, no need to do customized version testing ourselves and things are more declarative. Anyways, this PR has obviously already helped the wheels support be better :smiley: , so I'm OK if this is not wanted and go ahead to close.

yxliang01 commented 3 years ago

Thanks @Czaki for your review and comments, they are reasonable and helpful. However, I would wait for the maintainers' comments to keep this PR up-to-date and make further improvement. After all, I can also understand their concerns regarding increased encapsulation, so I think it's just a matter of preference.

inducer commented 2 years ago

Superseded by #559, I think. Thanks again for your work on this.