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 as a Python package #176

Closed alecandido closed 1 year ago

alecandido commented 1 year ago

The goal of this PR is just to add metadata for Python packaging. No new crate/folder is going to be created, because no further code should be added.

To do:

In order to test locally, you can use the usual:

Eventually, we will use maturin publish (possibly in the same workflow distributing pineappl_py), and just upload the further package pineappl-cli to PyPI, just wrapping the Rust binary. https://github.com/N3PDF/pineappl/blob/81dd719f62103895be245251af38582d05dca80b/.github/workflows/wheels.yml#L20 https://github.com/N3PDF/pineappl/blob/81dd719f62103895be245251af38582d05dca80b/.github/workflows/wheels.yml#L47

codecov[bot] commented 1 year ago

Codecov Report

Merging #176 (b536208) into master (81dd719) will increase coverage by 0.07%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   89.66%   89.74%   +0.07%     
==========================================
  Files          47       47              
  Lines        7337     7432      +95     
==========================================
+ Hits         6579     6670      +91     
- Misses        758      762       +4     
Flag Coverage Δ
python 80.64% <ø> (ø)
rust 89.86% <ø> (+0.07%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pineappl_cli/src/helpers.rs 87.42% <0.00%> (-0.08%) :arrow_down:
pineappl_cli/src/plot.rs 91.33% <0.00%> (-0.05%) :arrow_down:
pineappl/src/sparse_array3.rs 97.83% <0.00%> (+<0.01%) :arrow_up:
pineappl/src/import_only_subgrid.rs 95.27% <0.00%> (+0.01%) :arrow_up:
pineappl_cli/src/orders.rs 95.28% <0.00%> (+0.04%) :arrow_up:
pineappl/src/grid.rs 87.39% <0.00%> (+0.04%) :arrow_up:
pineappl_cli/src/remap.rs 98.48% <0.00%> (+0.04%) :arrow_up:
pineappl/src/bin.rs 91.89% <0.00%> (+0.05%) :arrow_up:
pineappl_cli/src/import/fktable.rs 97.27% <0.00%> (+0.05%) :arrow_up:
pineappl_cli/src/import/fastnlo.rs 86.13% <0.00%> (+0.05%) :arrow_up:
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cschwan commented 1 year ago

That's ingenious :+1:! We have to decide which features to activate; right now I suppose it uses the default features, which do not include the importers. However, supporting this might be difficult, unless we link everything together statically (which might be a good idea).

alecandido commented 1 year ago

unless we link everything together statically (which might be a good idea)

If the size of the final binary is no more than a few MB (even 10-20 MB I consider reasonable, more it starts being a compromise), I believe it would really be the best option.

cschwan commented 1 year ago

I just compiled it and it seems it ships LHAPDF!

cschwan commented 1 year ago

@AleCandido how do I install the binary after having it built with maturin build?

alecandido commented 1 year ago

@AleCandido how do I install the binary after having it built with maturin build?

I believe you just want something like this:

pip install ./downloads/SomeProject-1.0.4.tar.gz
alecandido commented 1 year ago

I just compiled it and it seems it ships LHAPDF!

Yes, but you know where it is coming from. It is simply a build time dependency, and then Rust compiles everything statically by default.

alecandido commented 1 year ago

Eventually, I would make this a dependency of the pineappl Python package.

Logically, it would be the other way round, but I believe to be easier to say: "Just do:

pip install pineappl

and you will get Python API + CLI out of the box".

I would not try instead to make a single bundle. I'm very happy about what maturin is providing us right now, so I prefer not to be too demanding.

cschwan commented 1 year ago

Yes, but you know where it is coming from. It is simply a build time dependency, and then Rust compiles everything statically by default.

If you open the wheel by unzipping it you can see that it ships libLHAPDF.so.

alecandido commented 1 year ago

If you open the wheel by unzipping it you can see that it ships libLHAPDF.so.

Ah, this is completely unexpected to me. Most likely depends on the way you packaged it, but still unexpected.

cschwan commented 1 year ago

Eventually, I would make this a dependency of the pineappl Python package.

Logically, it would be the other way round, but I believe to be easier to say: "Just do:

pip install pineappl

and you will get Python API + CLI out of the box".

I would not try instead to make a single bundle. I'm very happy about what maturin is providing us right now, so I prefer not to be too demanding.

I agree with everything you said.

cschwan commented 1 year ago

This looks good. We should build the CLI with all features and release mode,

maturin build --release --all-features

and ideally we should also install ROOT to support the conversion of .root APPLgrids. However, without a custom container that might be impossible.

alecandido commented 1 year ago

and ideally we should also install ROOT to support the conversion of .root APPLgrids. However, without a custom container that might be impossible.

What about a pre-compiled distribution?

In one case I actually have control over the container (and being Linux should be simple enough to have compatible binaries). In the other two cases we can do it as part of the job, so if binaries are available should be simple enough.

However, I would consider to support APPLgrid import even only in the simpler case of Linux binaries, and keep the Windows and MacOS distribution import-less (we don't need to make always the perfect job, some mild limitations are fine if they limit the amount of work).

alecandido commented 1 year ago

I merge this, since it's already working, and I'll manually release the first package. I will introduce the CI support and the pineappl package dependency in a later PR.