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

Merged
merged 1 commit into from
Sep 16, 2022
Merged

Distribute CLI as a Python package #176

merged 1 commit into from
Sep 16, 2022

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Sep 16, 2022

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:

  • add required metadata
  • prepare CI for release
  • add dependency on this new package in the pineappl Python package

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

  • maturin develop to build and install in current environment
  • maturin build to make the actual Python package, without installation

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

codecov bot commented Sep 16, 2022

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%) ⬆️

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%) ⬇️
pineappl_cli/src/plot.rs 91.33% <0.00%> (-0.05%) ⬇️
pineappl/src/sparse_array3.rs 97.83% <0.00%> (+<0.01%) ⬆️
pineappl/src/import_only_subgrid.rs 95.27% <0.00%> (+0.01%) ⬆️
pineappl_cli/src/orders.rs 95.28% <0.00%> (+0.04%) ⬆️
pineappl/src/grid.rs 87.39% <0.00%> (+0.04%) ⬆️
pineappl_cli/src/remap.rs 98.48% <0.00%> (+0.04%) ⬆️
pineappl/src/bin.rs 91.89% <0.00%> (+0.05%) ⬆️
pineappl_cli/src/import/fktable.rs 97.27% <0.00%> (+0.05%) ⬆️
pineappl_cli/src/import/fastnlo.rs 86.13% <0.00%> (+0.05%) ⬆️
... 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
Copy link
Contributor

cschwan commented Sep 16, 2022

That's ingenious 👍! 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
Copy link
Member Author

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

cschwan commented Sep 16, 2022

I just compiled it and it seems it ships LHAPDF!

@cschwan
Copy link
Contributor

cschwan commented Sep 16, 2022

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

@alecandido
Copy link
Member Author

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

alecandido commented Sep 16, 2022

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

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

cschwan commented Sep 16, 2022

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

alecandido commented Sep 16, 2022

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

cschwan commented Sep 16, 2022

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

cschwan commented Sep 16, 2022

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

alecandido commented Sep 16, 2022

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

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.

@alecandido alecandido merged commit 9166fb2 into master Sep 16, 2022
@alecandido alecandido deleted the pycli branch September 16, 2022 13:17
@alecandido alecandido mentioned this pull request Sep 16, 2022
2 tasks
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