Skip to content
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

Distribute CLI in a Python package #266

Merged
merged 93 commits into from
Jun 29, 2024
Merged

Distribute CLI in a Python package #266

merged 93 commits into from
Jun 29, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Mar 22, 2024

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 alecandido changed the base branch from master to nix March 22, 2024 03:46
@alecandido
Copy link
Member Author

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
Copy link
Member Author

Ok, now the problem is fully clear:

  • apparently, auditwheel started as a fork of delocate
  • they both shared the same problem of the patching the executables paths (not shared objects)
  • auditwheel patched after we encountered it in the previous PR (i.e. Distribute Python Packages #179)
    • I don't find the solution too elegant, but maybe there are good reasons to avoid doing it in different ways - and well... it's working
  • delocate still has the same problem

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
Copy link
Member Author

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

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

@alecandido
Copy link
Member Author

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.

👌
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
Copy link
Contributor

cschwan commented Jun 28, 2024

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
Copy link
Member Author

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

On my MacOS laptop (Apple Silicon)
: 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] <COMMAND>

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 <IDX>  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
Copy link
Contributor

cschwan commented Jun 29, 2024

OK, ideally commit e408b2b 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 Distribute Python Packages #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 295a275.

@cschwan cschwan marked this pull request as ready for review June 29, 2024 08:13
@alecandido
Copy link
Member Author

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
Copy link
Contributor

cschwan commented Jun 29, 2024

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

@cschwan cschwan merged commit 8665233 into master Jun 29, 2024
10 checks passed
@cschwan cschwan deleted the pycli branch June 29, 2024 13:01
This was referenced Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants