NNPDF / pineappl

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

Move python bindings #57

Closed alecandido closed 3 years ago

alecandido commented 3 years ago

Move python bindings into main pineappl crate

alecandido commented 3 years ago

First question: the obvious thing to do it's to port all the public methods of the required structs to python (and at the end of all the public structs), but of course some of them are not just working out of the box.

Should I do like for the objects, that I'm just implementing some of them for the time being, and so split the impl blocks in those that are going to collect the immediately pythonized objects and those that are waiting, or instead should I try to port all of them immediately?

cschwan commented 3 years ago

Before I answer your question, please use the cfg and cfg_attr attributes as shown in the last commit. Is it possible that I can't compile the entire crate yet?

alecandido commented 3 years ago

I'm trying, but unfortunately the answer was not already before. However I can tell you that now we have brand new problems:

  1. not a problem, just a remark, now you have to run like this:
    maturin develop --cargo-extra-args="--features python"
  2. the #[new] macro looks like it's not working in the new style:
    error: cannot find attribute `new` in this scope=====================> ] 81/82: pineappl
    --> pineappl/src/bin.rs:164:36
    |
    164 |     #[cfg_attr(feature = "python", new)]
    |                                    ^^^

    and same for #[pyclass] and #[pymethods]:

    
    error: cannot find attribute `pyclass` in this scope=================> ] 81/82: pineappl
    --> pineappl/src/grid.rs:178:32
    |
    178 | #[cfg_attr(feature = "python", pyclass)]
    |                                ^^^^^^^

error: cannot find attribute pymethods in this scope===============> ] 81/82: pineappl --> pineappl/src/grid.rs:189:32 | 189 | #[cfg_attr(feature = "python", pymethods)] | ^^^^^^^^^



Are you sure that `#[cfg_attr(...)]` is suitable for macros?
cschwan commented 3 years ago

As far as I understand it should work, see https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg_attr-attribute. For time being you can simply revert all changes.

cschwan commented 3 years ago

It seems that cfg_attr and new don't play well together: https://github.com/PyO3/pyo3/issues/780

alecandido commented 3 years ago

I would like to keep them, since I really like more the bindings organized this way than just spread all over, without any way to strip them off.

cschwan commented 3 years ago

Concerning your original question: I'd suggest that first tackle the most important methods and objects, just as in pineappl_py. We can then import more as soon as we need them.

Which methods don't work out of the box?

alecandido commented 3 years ago

It seems that cfg_attr and new don't play well together: PyO3/pyo3#780

I'm looking in the thread for hints, seems like not always is a pyo3 fault, even if I still don't fully understand:

@davidhewitt There are no errors with pymethods or pyclass because I forgot to enable the macros feature flag. I am sorry for the fuzz, should have paid more attention to the changelog.

Concerning the former question:

Concerning your original question: I'd suggest that first tackle the most important methods and objects, just as in pineappl_py. We can then import more as soon as we need them.

Fine, but actually I raised the question too early

Which methods don't work out of the box?

Indeed, they few methods in impl BinRemapper are actually all extremely simple, but the problems are with the types:

   --> pineappl/src/bin.rs:166:25
    |
166 |         normalizations: Vec<f64>,
    |                         ^^^ the trait `From<&PyCell<BinRemapper>>` is not implemented for `Vec<f64>`
    |

   ::: /home/alessandro/.cargo/registry/src/github.com-1ecc6299db9ec823/pyo3-0.12.4/src/callback.rs:170:8
    |
170 |     T: IntoPyCallbackOutput<U>,
    |        ----------------------- required by this bound in `pyo3::callback::convert`
    |

and so on, so it's not playing well with the following types:

that actually were perfectly fine (at least the first two) inside pineappl_py. So I'm looking for the solution...

cschwan commented 3 years ago

@AleCandido : that's strange, I'll have a look at it.

alecandido commented 3 years ago

Actually the referred option is described here. I wonder if we should add it explicitly: before it was working fine (and it is on the old branch), so I imagine that either pyo3 or maturin are already providing it somehow.

cschwan commented 3 years ago

I'm closing this since we can't use #[cfg_attr(feature = "python", pymethods)]. The current approach in #51 is the right one.

alecandido commented 3 years ago

Maybe there will be some chance later on: https://github.com/PyO3/pyo3/issues/780#issuecomment-825444650