Improve Python interface

cschwan commented 3 years ago

[x] remove old Python interface
[x] write github action for automatically uploading to PyPI whenever a release is published df7c8b556f006e91e091f86b54b958144c4aac88 - 88b78125e93addc0b753f714ff8533cfd6aa0572
[x] change convolute_eko to return FkTable instead of Grid
[x] ~~minimize the use of unwrap~~ I'm deferring this to a later stage
[x] improve test coverage
[x] make the old Python example working again 31ef6d3213bf625a814fea2a333846a914ad6d84
[x] publish Python documentation using .readthedocs.yml https://github.com/N3PDF/pineappl/commit/ed0c9d6680daa9b9e6318c4fd9d29137b60a770c
[x] improve pineappl_py/README.md

felixhekhorn commented 3 years ago

python test coverage mostly done in https://github.com/N3PDF/pineappl/commit/baa6e9247e03edf01d2d9574b27c6a2722dc10e0

alecandido commented 3 years ago

test coverage mostly done in baa6e92

Most likely @cschwan meant the coverage shown by the badge, and that takes only care about the rust files (see codecov).

cschwan commented 3 years ago

python test coverage mostly done in baa6e92

That's what I meant, the Rust code coverage is a separate concern.

alecandido commented 3 years ago

The better :)

felixhekhorn commented 3 years ago

change convolute_eko to return FkTable instead of Grid

I started to address this in https://github.com/N3PDF/pineappl/commit/75055fd9471a764d34e7d902e176fc0c4578afdd I'm not sure this is done, yet

alecandido commented 2 years ago

[ ] write github action for automatically uploading to PyPI whenever a release is published df7c8b5 - 88b7812

Just waiting to switch:

TestPyPI -> PyPI
workflow_dispatch -> on: release

in the workflow.

alecandido commented 2 years ago

We are now releasing even windows wheels with 54e9c55b83676a693f9d1256dcd8746ecd6df0cd

Now polishing the workflow, since looks like:

in order to get multiple cpython wheels on macOS you need to run maturin with different python interpreters available (so you need a matrix job in order to rerun a job for eachl cpython wanted)
on windows it behaves like manylinux: single job required to build many wheels (cpython 3.6, 3.7, 3.8, 3.9), setup python is not even required

alecandido commented 2 years ago

deploy-windows actually polished in a0b17fe3cbc15162a68f91517cccd4933e8ef03e

alecandido commented 2 years ago

[ ] make the old Python example working again 31ef6d3

I believe this has been solved in the commit I put by @felixhekhorn, can you confirm?

cschwan commented 2 years ago

[ ] make the old Python example working again 31ef6d3

I believe this has been solved in the commit I put by @felixhekhorn, can you confirm?

Yes, works flawlessly!

cschwan commented 2 years ago

What's the status of .readthedocs.yml? The badge in README.md says it's not working. Is it possible to put this file into the pineappl_py subfolder (where it belongs)?

alecandido commented 2 years ago

What's the status of .readthedocs.yml? The badge in README.md says it's not working. Is it possible to put this file into the pineappl_py subfolder (where it belongs)?

The one that is taking care of this is @felixhekhorn, but maybe I can reply to the second question: if we need to store any confiuration most likely no either root or not at all.

cschwan commented 2 years ago

[ ] write github action for automatically uploading to PyPI whenever a release is published df7c8b5 - 88b7812

Just waiting to switch:
* TestPyPI `->` PyPI

* `workflow_dispatch` `->` `on: release`
in the workflow.

Alright. In that case, @AleCandido please go ahead with the above, and I'll make a release soon!

cschwan commented 2 years ago

I fixed .readthedocs.yml in commits 3e57f893a6a7879e1a63581864d366270544a93a and 56815e767173578a9148c94c6cf9263d24ca777c so that it actually triggers the build, nevertheless it still fails later on: https://readthedocs.org/projects/pineappl/builds/14840270/. It seems that cargo is missing, @AleCandido @felixhekhorn do you have any idea how to fix it?

alecandido commented 2 years ago

I actually switched the workflow to target the official PyPI, now you can keep going @cschwan (we did whatever was possible to test it ahead of time, now I hope it should work out of the box). You should be able just to make a release (we already deployed current version on testPyPI, but the two are completely independent), but for consistency I would make a new tag and start with beta versions.

I'm going to investigate readthedocs, I understand the problem and I already looked for something similar, but I don't remember perfectly what I found...

alecandido commented 2 years ago

No way with readthedocs, see note about C dependencies, that is pointed by the generic FAQ on install dependencies.

Essentially on readthedocs only python (i.e. PyPI) dependencies are allowed.

The alternative is to switch docs on github pages, building them in a workflow (we'll put at something like https://n3pdf.github.io/pineappl/pydocs).

cschwan commented 2 years ago

I actually switched the workflow to target the official PyPI, now you can keep going @cschwan (we did whatever was possible to test it ahead of time, now I hope it should work out of the box). You should be able just to make a release (we already deployed current version on testPyPI, but the two are completely independent), but for consistency I would make a new tag and start with beta versions.

OK, in that case I'm going to release a beta version.

cschwan commented 2 years ago

No way with readthedocs, see note about C dependencies, that is pointed by the generic FAQ on install dependencies.

Essentially on readthedocs only python (i.e. PyPI) dependencies are allowed.

The alternative is to switch docs on github pages, building them in a workflow (we'll put at something like https://n3pdf.github.io/pineappl/pydocs).

I read the same pages, but there's support for Conda in RTD, and in conda-forge there's a Rust package: https://anaconda.org/conda-forge/rust. Can we use that? If not, we'll host the documentation ourselves.

cschwan commented 2 years ago

Seems to have worked: https://pypi.org/project/pineappl/0.5.0_beta/

cschwan commented 2 years ago

I managed to make a bit of progress using conda, see commit 4a2a8bba2fd70e186d9d167eac632a688bd8cecd, but the build still fails: https://readthedocs.org/projects/pineappl/builds/14842785/.

cschwan commented 2 years ago

Seems to have worked: https://pypi.org/project/pineappl/0.5.0_beta/

@AleCandido It doesn't seem to work on my new computer:

$ pip3 install --pre --user pineappl
Collecting pineappl
  Using cached https://files.pythonhosted.org/packages/fa/22/acabc45e0d29ea12b7eccdb62025f1b082931d8f054b296d643687996d52/pineappl-0.5.0_beta.tar.gz
  Installing build dependencies ... done
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/usr/lib/python3.7/tokenize.py", line 447, in open
        buffer = _builtin_open(filename, 'rb')
    FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pip-install-z8w4889t/pineappl/setup.py'

Is it possible that it is a problem with Python 3.7 (if it's, let's not worry about it)? It works on montblanc (Python 3.8) and dom (Python 3.9).

cschwan commented 2 years ago

I managed to make a bit of progress using conda, see commit 4a2a8bb, but the build still fails: https://readthedocs.org/projects/pineappl/builds/14842785/.

That's probably the following Issue: https://github.com/PyO3/maturin/issues/330.

felixhekhorn commented 2 years ago

agreed - can you trick some additional pip arguments into path?

https://github.com/N3PDF/pineappl/blob/4a2a8bba2fd70e186d9d167eac632a688bd8cecd/.readthedocs.yml#L11

alecandido commented 2 years ago

Maybe you're right and it's just possible with conda, but unfortunately I'm not familiar with it. I will have a look myself in the next few days, if possible let's try to make it work, otherwise gh-pages is always an option.

cschwan commented 2 years ago

@AleCandido I'm pretty sure that @felixhekhorn and I solved the problem now. We just need an environment variable PIP_USE_FEATURE=in-tree-build in the readthedocs configuation of this repository, and then it should work. However, @scarrazza has to do it because we don't have the rights.

cschwan commented 2 years ago

We get a different build error now, something is wrong with sphinx: https://readthedocs.org/projects/pineappl/builds/14844451/.

felixhekhorn commented 2 years ago

my bad - fixed in https://github.com/N3PDF/pineappl/commit/bf121b6821e6ad977770070680415e71bb3b8bb5

cschwan commented 2 years ago

There are more errors; apparently it can't find bibtex: https://readthedocs.org/projects/pineappl/builds/14844604/.

felixhekhorn commented 2 years ago

it's because it is not reading the requirements file for some reason ... I tried some trival rename here https://github.com/N3PDF/pineappl/commit/6119730e26a89a9f5431e7b3307d37760e0b8fbc

here you can read [u'/home/docs/checkouts/readthedocs.org/user_builds/pineappl/conda/latest/bin/python', '-m', 'pip', 'install', '-U', '-r', '/home/docs/checkouts/readthedocs.org/user_builds/pineappl/checkouts/latest/pineappl_py/tmpS2bWML.requirements.txt'] and that seems odd to me, so I tried a simple rename

the required package shinxcontrib-bibtex is indeed listed here: https://github.com/N3PDF/pineappl/blob/6119730e26a89a9f5431e7b3307d37760e0b8fbc/pineappl_py/devel-requirements.txt#L6

felixhekhorn commented 2 years ago

@cschwan @AleCandido :tada: went through

felixhekhorn commented 2 years ago

@cschwan @AleCandido tada went through

sigh I was so happy I ticked the item prematurely - this is not working (yet) we're missing all the PyO3 stuff, i.e. the pineappl.pineappl module ... https://pineappl.readthedocs.io/en/latest/modules/pineappl/pineappl.html

felixhekhorn commented 2 years ago

I'm giving up for today ... we need to fix

💥 maturin failed
  Caused by: You need to be inside a virtualenv or conda environment to use develop (neither VIRTUAL_ENV nor CONDA_PREFIX are set). See https://virtualenv.pypa.io/en/latest/index.html on how to use virtualenv or use `maturin build` and `pip install <path/to/wheel>` instead.

here

and we do have a conda env around ...

I'm also slightly concerned that we're too late - because we're already inside sphinx

alecandido commented 2 years ago

I managed to make a bit of progress using conda, see commit 4a2a8bb, but the build still fails: https://readthedocs.org/projects/pineappl/builds/14842785/.

That's probably the following Issue: https://github.com/PyO3/maturin/issues/330.

Sorry not to have followed updates, I will check everything tomorrow.

Just to make some progress: I'm pretty sure the reason might be different, because in the quoted issue the problem is with the build pipeline triggered by pip install ., indeed pip is triggering the build system looking into pyproject.toml, and then there will be a problem with relative paths handling there. Instead we're directly using maturin, and fortunately nothing happened.

In your case all the build has already happened somewhere else (in the github workflow), and then the problem is not to make a binary wheel, but to resolve and install an existing one. Could you share some details on your new system? (I'm also curious why your new computer is coming with python3.7, but for me it is good since you're testing another corner)

alecandido commented 2 years ago

I'm giving up for today ... we need to fix
💥 maturin failed
  Caused by: You need to be inside a virtualenv or conda environment to use develop (neither VIRTUAL_ENV nor CONDA_PREFIX are set). See https://virtualenv.pypa.io/en/latest/index.html on how to use virtualenv or use `maturin build` and `pip install <path/to/wheel>` instead.
here

and we do have a conda env around ...

I'm also slightly concerned that we're too late - because we're already inside sphinx

If you read a few lines above there is the answer to your doubt:

CommandNotFoundError: Your shell has not been properly configured to use 'conda activate'.
To initialize your shell, run

    $ conda init <SHELL_NAME>

Meaning: maturin is failing because conda failed in before. Why conda failed, this I don't know (yet)

EDIT: also consider the following warning from conda:

Warning: you have pip-installed dependencies in your environment file, but you do not list pip itself as one of your conda dependencies.  Conda may not use the correct pip to install your packages, and they may end up in the wrong place.  Please add an explicit pip dependency.  I'm adding one for you, but still nagging you.

alecandido commented 2 years ago

A proposal solution for the RTD issue might be the following: since environment.yml contains just a few dependencies

https://github.com/N3PDF/pineappl/blob/716abc8a0e628575fb134d0fc7c3b90b6d8656d1/pineappl_py/environment.yml#L4-L7

we can replace them with apt and pip (maybe):

sphinxcontrib-bibtex and maturin have PyPI packages we can just use pip
for rust (and even cargo) there should be ubuntu packages

Then maybe we can skip conda at all.

cschwan commented 2 years ago

I can read the documentation now, so does it mean it's working?

cschwan commented 2 years ago

In your case all the build has already happened somewhere else (in the github workflow), and then the problem is not to make a binary wheel, but to resolve and install an existing one. Could you share some details on your new system? (I'm also curious why your new computer is coming with python3.7, but for me it is good since you're testing another corner)

It's a debian 10 machine, which isn't too old. Debian 11 (with Python 3.9) has only been released last month. How much work is it to add Python 3.7 support?

felixhekhorn commented 2 years ago

I can read the documentation now, so does it mean it's working?

unfortunately no ...

sigh I was so happy I ticked the item prematurely - this is not working (yet) we're missing all the PyO3 stuff, i.e. the pineappl.pineappl module ... https://pineappl.readthedocs.io/en/latest/modules/pineappl/pineappl.html

alecandido commented 2 years ago

As you said Debian 10 is quite common, I was not aware it was still on 3.7 (I would have expected 3.8, but that's not so strange considering it's Debian).

And the problem is that it should work, already, without any extra effort (just pip install --pre pineappl). Since it's not the case we have to understand why it's not working, and I'm not sure it's 3.7 fault.

cschwan commented 2 years ago

I can read the documentation now, so does it mean it's working?

unfortunately no ...

sigh I was so happy I ticked the item prematurely - this is not working (yet) we're missing all the PyO3 stuff, i.e. the pineappl.pineappl module ... https://pineappl.readthedocs.io/en/latest/modules/pineappl/pineappl.html

I probably don't understand because I'm not too familiar with Python, what exactly is missing here?

cschwan commented 2 years ago

[..] (I would have expected 3.8, but that's not so strange considering it's Debian).

:grin:

And the problem is that it should work, already, without any extra effort (just pip install --pre pineappl). Since it's not the case we have to understand why it's not working, and I'm not sure it's 3.7 fault.

Is it possible that there's interference with Python 2? It's still the default on Debian 10...

alecandido commented 2 years ago

@cschwan write here your pip version and then try to upgrade it pip install --upgrade pip

Seems like the issue is known on SO

felixhekhorn commented 2 years ago

I probably don't understand because I'm not too familiar with Python, what exactly is missing here?

we're missing the PyO3 module, i.e. e.g. the documentation for PyBinRemapper (that is written in Rust)

cschwan commented 2 years ago

@AleCandido Indeed, that was the problem. My pip version was:

pip 18.1 from /usr/lib/python3/dist-packages/pip (python 3.7)

It works with 21.2.4!

cschwan commented 2 years ago

I probably don't understand because I'm not too familiar with Python, what exactly is missing here?

we're missing the PyO3 module, i.e. e.g. the documentation for PyBinRemapper (that is written in Rust)

Do we really want to document it? The user should not be able to directly work with the Rust parts, in fact they shouldn't know that there's Rust involved at all. But maybe we'd like to have it as internal documentation (developer documentation)?

felixhekhorn commented 2 years ago

Do we really want to document it? The user should not be able to directly work with the Rust parts, in fact they shouldn't know that there's Rust involved at all. But maybe we'd like to have it as internal documentation (developer documentation)?

No, we need it because it holds all the crucial functions that are exposed to the user (remember our magic binding in PyWrapper) and hence we have to document it (and maturin is providing the documentation from below ...)

e.g. PyGrid.bin_left()

alecandido commented 2 years ago

@AleCandido Indeed, that was the problem. My pip version was:
pip 18.1 from /usr/lib/python3/dist-packages/pip (python 3.7)
It works with 21.2.4!

So bad...

It's true that there are 3 majors between 18 and 21, and each one might break compatibility, but pip is iterating too fast on majors...

cschwan commented 2 years ago

Do we really want to document it? The user should not be able to directly work with the Rust parts, in fact they shouldn't know that there's Rust involved at all. But maybe we'd like to have it as internal documentation (developer documentation)?

No, we need it because it holds all the crucial functions that are exposed to the user (remember our magic binding in PyWrapper) and hence we have to document it (and maturin is providing the documentation from below ...)

e.g. PyGrid.bin_left()

In that case what do we think about explicitly wrapping every method to document them?

cschwan commented 2 years ago

@AleCandido @felixhekhorn Can you please review commits 2665c9bd550b22abb1d100b826cc21c600084804, 73b6a5fa2894ab70ebd7c360e434a14d769a96b9 and 43f39e3efee39d454d0b73c6d29a96e67672bdb6? These changes are needed by @scarlehoff. I'll do another beta release shortly after I've got your OK.

felixhekhorn commented 2 years ago

@scarlehoff we also implemented the FkTable.read that we were talking about in https://github.com/N3PDF/pineappl/commit/a12338a845baf58678344f833c2aa7dfd46455c3 so you should use that one to read the FkTable

NNPDF / pineappl

Improve Python interface #71