NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Distribute CLI in a Python package #266

Closed alecandido closed 6 days ago

alecandido commented 3 months ago

Supersedes #179

Unfortunately, it is definitely not possible to use the provided maturin-action, because of missing libraries.

I will try to use maturin in the CI, making use of the pineappl-ci container, and the related script.sh for the builds that should happen on the host.

However, I should first solve the problem of dispatching libraries in the wheel. It seemed it was happening automatically before, or I forgot some steps...

alecandido commented 3 months ago

maturin is running auditwheel only on Linux, apparently.

It is possible to obtain the same results on MacOS and Windows (or it should be), but we have to do it "manually", i.e. by using different projects: https://github.com/pypa/auditwheel https://github.com/matthew-brett/delocate https://github.com/adang1345/delvewheel

alecandido commented 3 months ago

Ok, now the problem is fully clear:

After confirming the auditwheel repaired wheel is working, I will fork delocate and apply the same patch. In principle, there is no reason for the patch not to work on delvewheel as well, extending the compatibility to windows. But I've no way to test it, so I will leave it to someone else (and until there, there won't be PineAPPL CLI support for Windows).

The executable issue

The path that is patched by delocate (and consequently the others) is actually correct inside the wheel itself.

The problem results from a "quirk" of the wheel installation. While the content of the wheel is mostly just unpacked in platlib, the executables are instead moved to $PREFIX/bin ($PREFIX == $VIRTUAL_ENV, unless user or system installation). While this is a sensible behavior, we have to instruct our patching tools that this is going to happen.

alecandido commented 3 months ago

The patch mentioned is very simple, and it's just the following:

https://github.com/pypa/auditwheel/pull/443/files#diff-577457911632c6ec0ef425fb8b518d41422610625f86c5027ff2be2012ccf80c

alecandido commented 3 months ago

@cschwan @scarlehoff @felixhekhorn Report on present status.

TL;DR: the more it's useless, the better is working

I suspect that the patching part of the wheels generation is working. The script that I've made, porting from the auditwheel patch mentioned above, it is actually generating the expected output, I tested with locally generated wheels.

The workflow "itself" is already working (it is capable of building PineAPPL and patching wheels, given some *conditions**), as it is proven by the case of MacOS x86_64 (that is passing and generating wheels).

*And here on the conditions:

Windows

Most likely this is the most useless, but it should be even the simplest to fix: I'm just missing autotools for the time being, so I need a shell with the same tools I have on Mac and Linux. Once LHAPDF will be compiled, (I hope) maturin-action will find it and compile the CLI as it is able to compile the library crate.

Linux

This is pretty complex: maturin-action, as cibuildwheel, is essentially running the build in a container. Thus, it is useless to build LHAPDF on the host (that would be simply, now). I tried to run the whole job in a manylinux container, but it is quite a mess (Node 20 actions are broken, so we have to keep using older actions, and potentially installing a bunch of missing tools - I also tried with pineappl-ci, but didn't work till the end).

Luckily, I found that maturin-action allows running a script before executing the build. I tried to build LHAPDF in there, but apparently I didn't find the way to compile C++ code 🤦 https://github.com/NNPDF/pineappl/actions/runs/8447503668/job/23137971827?pr=266#step:3:275

MacOS - aarch64

This could be the most complicate (or it ties with the previous one), since maturin-action relies on cargo cross-compilation (I believe), and it works perfectly for pineappl_py. However, building the dependency on the host means that it will be x86_64, and this is currently breaking the build (since it's the library is missing for aarch64).

I tried to use the newer runner macos-14, and make a build of LHAPDF in there. Tools are missing, Python versions as well, and it's a giant mess...

If anyone would like to take over and make some attempts, it would be very much appreciated.

alecandido commented 1 week ago

This still contains pieces of #245, but I changed base branch, because https://github.com/NNPDF/pineappl/tree/nix is not up-to-date, and otherwise the "Files changed" tab and the list of commits were completely useless.

cschwan commented 1 week ago

Thanks to static linking we are now able to generate CLI wheels for Linux and macOS. I also made some progress for Windows, but the problem there is that pkg-config from inside Rust isn't found, probably because Rust sees the Windows filesystem, while in the shell the paths all appear Linux-like. @alecandido do you have any idea how the two are connected?

I also shifted the CLI-wheels generation into the Release workflow. Something that's missing is to enable all features of the CLI. For the time being only --features=static is enabled. Finally, we'd need to publish the wheels (we already use https://pypi.org/project/pineappl-cli/), but I don't know how to do that (yet).

alecandido commented 1 week ago

Thanks to static linking we are now able to generate CLI wheels for Linux and macOS.

Great!

I also made some progress for Windows, but the problem there is that pkg-config from inside Rust isn't found, probably because Rust sees the Windows filesystem, while in the shell the paths all appear Linux-like.

Starting (once in a lifetime) from the dirty solution first: we're using Bash on Windows, but in case it's needed, Powershell is also available as pwsh. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#defaultsrunshell

However, the problem is not caused by bare Rust, but rather by the maturin-action https://github.com/PyO3/maturin-action/blob/2c5c1560848aaa364c3545136054932db5fa27b7/src/index.ts#L839-L964 I believe the action to run in the Node runtime on the host (the one also running the whole Workflows infrastructure, most likely) and it's getting the Rust and Maturin binaries directly from Windows. Instead, I believe the shell is running within MSYS2, as flagged by the filesystem path https://www.msys2.org/docs/environments/

That's why resolving the binary in the Bash shell, and trying to use it within Rust within the Maturin action, will be quite hard, if not impossible.

A "clean" solution would be to access the Windows file system from Bash, to keep the code similar to the other hosts, and having scripts in a language that is familiar to most of the PineAPPL developers. But I'm not even sure if you can escape MSYS2, that's why I started from the "dirty" one.

alecandido commented 1 week ago

I also shifted the CLI-wheels generation into the Release workflow.

I see. But is it a good idea to keep such a large workflow file? Did you know that you can also call workflows in other files from a workflow? (without having to make an action for that) https://docs.github.com/en/actions/using-workflows/reusing-workflows It is mainly made for reusing workflows (and we're also using them in that way, https://github.com/NNPDF/workflows), but it's not bad even to make the workflow files more modular.

Something that's missing is to enable all features of the CLI. For the time being only --features=static is enabled.

Ah, yes. I didn't bother myself linking to fastNLO and APPLgrid, but you can do via static or dynamic linking as well. Dynamic linking means going through the pain of distributing those libraries in the wheel once again (with all the niceties of auditwheel and friends), but not sure how simpler is to statically link them. However, I don't see it as a real priority: the CLI is mainly done for working with PineAPPL grids, so it's fair to have a simple distribution that just works with those. If you need to convert, you could always compile it once in a while, convert your grids, and then go back using the usual one wherever you wish. It should be alright to consider it an advanced use case (at least for some time, if not forever).

Finally, we'd need to publish the wheels (we already use pypi.org/project/pineappl-cli), but I don't know how to do that (yet).

That's pretty simple. You could that manually with twine, or using the specific GitHub action https://github.com/marketplace/actions/pypi-publish. They are both official tools by the PyPA, and I advise the second as more specific. In principle, everything is happening under the hood with a Web API, just sending requests with some files to upload. But I see no reason to reimplement the upload manually, while there are official tools doing that. Here an example of how we're using it (as anyone else is doing): https://github.com/NNPDF/workflows/blob/e681e4d636ae8000bd806e8124119c180dd7c6b7/.github/workflows/python-poetry-pypi.yml#L70-L73

To authenticate there is already a token in the secrets. I'm not sure whether it has a limited scope (if I generated it, almost for sure), but it may already span the name we need, since I squatted for it: https://pypi.org/project/pineappl-cli/ If needed, we could generate a new one. Just @scarlehoff for the access to https://pypi.org/user/N3PDF/

cschwan commented 1 week ago

I also shifted the CLI-wheels generation into the Release workflow.

I see. But is it a good idea to keep such a large workflow file?

Yes and no. Logically it's all related to making a release so for me it was 'the right thing' to do, but obviously we can change that again. I like that the release workflow is automatically triggered whenever I upload a release tag, but it's also manually triggerable to check whether it still works. In that case all the build products aren't published of course. At the end of the day, however, this workflow is extremely complicated and difficult to understand.

There's also the potential for making it simpler, because for instance the cli-linux and cli-wheel also do exactly the same, only the packaging is different.

Did you know that you can also call workflows in other files from a workflow? (without having to make an action for that) https://docs.github.com/en/actions/using-workflows/reusing-workflows It is mainly made for reusing workflows (and we're also using them in that way, https://github.com/NNPDF/workflows), but it's not bad even to make the workflow files more modular.

I didn't! Or I forgot. I've got to study these manuals at point, there's probably cool stuff that one code that I don't know about yet.

Something that's missing is to enable all features of the CLI. For the time being only --features=static is enabled.

Ah, yes. I didn't bother myself linking to fastNLO and APPLgrid, but you can do via static or dynamic linking as well. Dynamic linking means going through the pain of distributing those libraries in the wheel once again (with all the niceties of auditwheel and friends), but not sure how simpler is to statically link them. However, I don't see it as a real priority: the CLI is mainly done for working with PineAPPL grids, so it's fair to have a simple distribution that just works with those. If you need to convert, you could always compile it once in a while, convert your grids, and then go back using the usual one wherever you wish. It should be alright to consider it an advanced use case (at least for some time, if not forever).

It should be very simple, because we basically need to copy the workflows of cli-linux and cli-macos. These where not easy to write, see https://github.com/NNPDF/pineappl/pull/269. I believe the grid conversion functionality is an important feature, because many people use APPLgrid and fastNLO, of course.

Finally, we'd need to publish the wheels (we already use pypi.org/project/pineappl-cli), but I don't know how to do that (yet).

That's pretty simple. You could that manually with twine, or using the specific GitHub action https://github.com/marketplace/actions/pypi-publish. They are both official tools by the PyPA, and I advise the second as more specific. In principle, everything is happening under the hood with a Web API, just sending requests with some files to upload. But I see no reason to reimplement the upload manually, while there are official tools doing that. Here an example of how we're using it (as anyone else is doing): https://github.com/NNPDF/workflows/blob/e681e4d636ae8000bd806e8124119c180dd7c6b7/.github/workflows/python-poetry-pypi.yml#L70-L73

To authenticate there is already a token in the secrets. I'm not sure whether it has a limited scope (if I generated it, almost for sure), but it may already span the name we need, since I squatted for it: https://pypi.org/project/pineappl-cli/ If needed, we could generate a new one. Just @scarlehoff for the access to https://pypi.org/user/N3PDF/

Thank you for the pointers, they're helpful. I just noticed that we can probably reuse the release-wheel job, which already does that for the Python interface.

cschwan commented 1 week ago

Starting (once in a lifetime) from the dirty solution first: we're using Bash on Windows, but in case it's needed, Powershell is also available as pwsh. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#defaultsrunshell

However, the problem is not caused by bare Rust, but rather by the maturin-action https://github.com/PyO3/maturin-action/blob/2c5c1560848aaa364c3545136054932db5fa27b7/src/index.ts#L839-L964 I believe the action to run in the Node runtime on the host (the one also running the whole Workflows infrastructure, most likely) and it's getting the Rust and Maturin binaries directly from Windows. Instead, I believe the shell is running within MSYS2, as flagged by the filesystem path https://www.msys2.org/docs/environments/

That's why resolving the binary in the Bash shell, and trying to use it within Rust within the Maturin action, will be quite hard, if not impossible.

A "clean" solution would be to access the Windows file system from Bash, to keep the code similar to the other hosts, and having scripts in a language that is familiar to most of the PineAPPL developers. But I'm not even sure if you can escape MSYS2, that's why I started from the "dirty" one.

I will try a bit more to make it work, just because we put so much work into it, it would be a shame to just throw away. More importantly, I know at least two people who use Windows who would profit from this. But let's see if it works.

cschwan commented 1 week ago

Oh, one thing that I forgot to ask, @alecandido: do you know if a Python wheel can also install manpages?

alecandido commented 1 week ago

I didn't! Or I forgot. I've got to study these manuals at point, there's probably cool stuff that one code that I don't know about yet.

Whether it's a cool is kind of debatable, the CI implementation (the runner) is proprietary and apparently convoluted (and hardly maintainable). But this is GitHub's business... Our business is that testing the CI is a pain in the neck, and sooner we should come back testing this: https://github.com/nektos/act

It should be very simple, because we basically need to copy the workflows of cli-linux and cli-macos. These where not easy to write, see #269. I believe the grid conversion functionality is an important feature, because many people use APPLgrid and fastNLO, of course.

:ok_hand: It's just a matter of priorities and available person power.

Thank you for the pointers, they're helpful. I just noticed that we can probably reuse the release-wheel job, which already does that for the Python interface.

Indeed, something like that. The action is pretty straightforward.

I will try a bit more to make it work, just because we put so much work into it, it would be a shame to just throw away. More importantly, I know at least two people who use Windows who would profit from this. But let's see if it works.

Thanks for your effort. I'd also like to see it, it'd be the perfect distribution for PineAPPL.

Oh, one thing that I forgot to ask, @alecandido: do you know if a Python wheel can also install manpages?

I'm pretty sure you can not, or not in the sense you have in mind. Python wheels are just zipped folders, which installation procedure is "unzip", and that's almost it. The installer is doing a minor amount of further operations, like moving the binaries in a discoverable location. But not much more, to the best of my understanding.

I believe the standard is drifting towards document CLIs through -h/--help, or even dedicated subcommands (like pineappl help), to avoid the installation burden. However, you can always check on each CLI invocation if they are installed or not, and in case install them (copying from the package). With a very minor overhead, and a lot of portability burden...

cschwan commented 1 week ago

OK, building the CLI works now both on Linux and macOS with all features. If you like to give it a try, @alecandido, download the artifacts from this page: https://github.com/NNPDF/pineappl/actions/runs/9713856339, called cli-wheel-*. The only thing that doesn't work is the conversion of .root APPLgrids, because then we'd need to link ROOT statically. I tried that once and I'm not sure this is possible or even a good idea. The only way to convert those is still to build the binary yourself.

alecandido commented 1 week ago

I didn't benchmark the results, but other than that it worked out of the box! 🎉 🎊

On my MacOS laptop (Apple Silicon) ```sh ❯ : pip install pineappl_cli-0.7.4-cp311-cp311-macosx_11_0_arm64.whl Processing ./pineappl_cli-0.7.4-cp311-cp311-macosx_11_0_arm64.whl Installing collected packages: pineappl-cli Successfully installed pineappl-cli-0.7.4 [notice] A new release of pip is available: 24.0 -> 24.1 [notice] To update, run: pip install --upgrade pip donaldville in qibolab/pine on  propagate-serialization [?] via 🐍 v3.11.9 (qibolab-py3.11) via ❄ impu re (devenv-shell-env) ❯ : pineappl Read, write, and query PineAPPL grids Usage: pineappl [OPTIONS] Commands: analyze Perform various analyses with grids channels Shows the contribution for each partonic channel convolve Convolutes a PineAPPL grid with a PDF set diff Compares the numerical content of two grids with each other evolve Evolve a grid with an evolution kernel operator to an FK table export Converts PineAPPL grids to APPLgrid files help Display a manpage for selected subcommands import Converts APPLgrid/fastNLO/FastKernel files to PineAPPL grids merge Merges one or more PineAPPL grids together orders Shows the predictions for all bin for each order separately plot Creates a matplotlib script plotting the contents of the grid pull Calculates the pull between two different PDF sets read Read out information of a grid subgrids Print information about the internal subgrid types uncert Calculate scale and convolution function uncertainties write Write a grid modified by various operations Options: --lhapdf-banner Allow LHAPDF to print banners --force-positive Forces negative PDF values to zero --allow-extrapolation Allow extrapolation of PDFs outside their region of validity --use-alphas-from Choose the PDF/FF set for the strong coupling [default: 0] -h, --help Print help -V, --version Print version donaldville in qibolab/pine on  propagate-serialization [?] via 🐍 v3.11.9 (qibolab-py3.11) via ❄ impu re (devenv-shell-env) ❯ : wget https://github.com/NNPDF/pineapplgrids/raw/master/LHCB_DY_7TEV.pineappl.lz4 --2024-06-29 00:03:24-- https://github.com/NNPDF/pineapplgrids/raw/master/LHCB_DY_7TEV.pineappl.lz4 Resolving github.com (github.com)... 140.82.121.3 Connecting to github.com (github.com)|140.82.121.3|:443... connected. HTTP request sent, awaiting response... 302 Found Location: https://media.githubusercontent.com/media/NNPDF/pineapplgrids/master/LHCB_DY_7TEV.pineappl.lz4 [following] --2024-06-29 00:03:24-- https://media.githubusercontent.com/media/NNPDF/pineapplgrids/master/LHCB_DY_7TEV.pineappl.lz4 Resolving media.githubusercontent.com (media.githubusercontent.com)... 2606:50c0:8002::154, 2606:50c0:8000::154, 2606:50c0:8001::154, ... Connecting to media.githubusercontent.com (media.githubusercontent.com)|2606:50c0:8002::154|:443... connected. HTTP request sent, awaiting response... 200 OK Length: 2651929 (2,5M) [application/octet-stream] Saving to: ‘LHCB_DY_7TEV.pineappl.lz4’ LHCB_DY_7TEV.pineappl.lz4 100%[=====================================>] 2,53M --.-KB/s in 0,1s 2024-06-29 00:03:25 (19,8 MB/s) - ‘LHCB_DY_7TEV.pineappl.lz4’ saved [2651929/2651929] donaldville in qibolab/pine on  propagate-serialization [?] via 🐍 v3.11.9 (qibolab-py3.11) via ❄ impu re (devenv-shell-env) ❯ : pineappl convolve LHCB_DY_7TEV.pineappl.lz4 CT10 b yll dsig/dyll [] [pb] --+-----+-----+------------ 0 2 2.125 6.8161988e0 1 2.125 2.25 1.9686682e1 2 2.25 2.375 3.1367695e1 3 2.375 2.5 4.1805491e1 4 2.5 2.625 5.0276912e1 5 2.625 2.75 5.6466578e1 6 2.75 2.875 6.0112492e1 7 2.875 3 6.0884635e1 8 3 3.125 5.8721908e1 9 3.125 3.25 5.3426156e1 10 3.25 3.375 4.4304837e1 11 3.375 3.5 3.2921399e1 12 3.5 3.625 2.2220291e1 13 3.625 3.75 1.3296498e1 14 3.75 3.875 6.8639432e0 15 3.875 4 2.8308688e0 16 4 4.25 5.3394142e-1 ```
cschwan commented 1 week ago

OK, ideally commit e408b2b61038de69c11c733692c74495bceb5bf3 takes care of the publishing. @alecandido, do you think this will work? I wonder if two things are correct:

  1. when I created an account at https://test.pypi.org I had to give the name of workflow file that uploads the package (actually I didn't do anything with the account, just wanted to see how it works), and for CLI wheels that supposedly changed from wheels.yml (in #179) to release.yml. Do you think this has an impact?
  2. Furthermore, we'll now upload two different wheels (pineappl and pineappl-cli) using the same maturin action.

I could merge this PR and make prelease to test it, but I wonder whether there's a better idea.

The Windows wheels have lower priority as its not quite clear if that works at all, but I kept the workflow file as cli-wheels-windows.yml in commit 295a2755f2ff8c948d4b48b1fb5be15b3f766fd6.

alecandido commented 1 week ago

Concerning 1. I'm definitely surprised, it's the first time I hear something like the platform asking for the workflow name. I already had an account on both https://test.pypi.org and https://pypi.org, and if I go there it only asks me to enable 2FA (to the point that it's denying all operations otherwise), and then I can generate an API token, with or without a specific scope.

Token generation image

To the best of my understanding, the only thing that should have an impact is the scope of the token you're using, and, in case it's not "Entire account (all projects)" which projects you selected.

Regarding 2., maybe there would be no problem, because maturin is determining the name of the project from the name of the wheel. But since the token can only be scoped to either the whole account (which I'm almost sure I avoided) and individual projects, then pineappl and pineappl-cli are two different projects (from PyPI perspective), thus we'll need two different tokens. And since you can only pass one at a time, most likely we'll need two invocations, filtering the wheels.

Moreover, I'd consider using the PyPA action. Maturin is a great project, used by many people, and well-maintained (mainly by only two people, but for the time being definitely reliable). However, I believe that maturin upload is a byproduct of maturin publish, which compiles and uploads in one shot. But I'm a huge fan of modularity ("one tool for one task"), and Maturin's core business is compiling wheels - thus I consider PyPA publishing tools more reliable. Though, we're talking about a pretty simple task, so it doesn't really make the difference.

I could merge this PR and make prelease to test it, but I wonder whether there's a better idea.

Unfortunately, the alternatives are unsatisfactory: you could manually trigger, but commenting all the filters for release events. Or locally test, attempting to reproduce the actions. Or use https://github.com/nektos/act, possibly making an effort to make it work (and possibly failing). I would say: make a pre-release. If you prefer, you could even tag a commit on this branch, and pre-release that, without merging first. It will be the best test. (but before, you should split the upload, and generate a token for pineappl-cli - in the process I'd rename the secrets in LIB-PYPI-TOKEN and CLI-PYPI-TOKEN, or something like that)

cschwan commented 6 days ago

It works: https://pypi.org/project/pineappl-cli/0.8.0a4/! I'm merging this now.