PyCV -- calling python within plumed

Iximiel commented 7 months ago

Description

See #996 for a more complete description: I have "rebased with cp" the pycv branch on the master branch

Target release

I would like my code to appear in release 2.10

Type of contribution

[x] changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
[ ] changes to a module not authored by you
[ ] new module contribution or edit of a module authored by you

Copyright

[x] I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.

[ ] the module I added or modified contains a COPYRIGHT file with the correct license information. Code should be released under an open source license. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.

Tests

[x] I added a new regtest or modified an existing regtest to validate my changes.
[ ] I verified that all regtests are passed successfully on GitHub Actions.

codecov-commenter commented 7 months ago

Codecov Report

Attention: 94 lines in your changes are missing coverage. Please review.

Comparison is base (5cbb885) 84.09% compared to head (916486d) 84.16%. Report is 22 commits behind head on master.

:exclamation: Current head 916486d differs from pull request most recent head a1bb90c. Consider uploading reports for the commit a1bb90c to get more accurate results

Files	Patch %	Lines
src/core/PlumedMain.cpp	88.20%	23 Missing :warning:
src/core/ActionWithArguments.cpp	60.60%	13 Missing :warning:
src/core/DomainDecomposition.cpp	95.10%	12 Missing :warning:
src/core/DataPassingObject.cpp	83.58%	11 Missing :warning:
src/core/ActionToPutData.cpp	92.39%	7 Missing :warning:
src/core/ActionToGetData.cpp	76.92%	6 Missing :warning:
src/core/DataPassingTools.cpp	81.81%	4 Missing :warning:
src/core/GREX.cpp	50.00%	4 Missing :warning:
src/core/ActionAtomistic.cpp	97.70%	3 Missing :warning:
src/adjmat/Sprint.cpp	50.00%	2 Missing :warning:
... and 7 more

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1001 +/- ## ========================================== + Coverage 84.09% 84.16% +0.07% ========================================== Files 602 612 +10 Lines 56233 56476 +243 ========================================== + Hits 47287 47531 +244 + Misses 8946 8945 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GiovanniBussi commented 7 months ago

@Iximiel I am trying to make this branch work on my laptop. Meanwhile, just looking at the scripts:

you are using in many places python3-config. This should be customizable. Thinking about the possible use cases:
- By default, use the variable python_bin configured in Makefile.conf. It's not trivial to access to Makefile variables from a shell script. My suggestion is to use a Makefile that includes Makefile.conf and then, if you need a shell script, pass the value of the relevant Makefile variables as arguments to the shell script or as environment variables. You can also get some idea from what we do here.
- To allow a user to just compile this plugin after plumed has been installed already somewhere, I guess you should allow the user to pass the name of the python interpreter likely as an environment variable. The variable could be PYTHON_BIN, consistently with what we use for ./configure
--no-as-needed is not available on MacOS. I guess it's not needed.

GiovanniBussi commented 7 months ago

Some fixes here.

A few things are missing. For the "simple" user, which would like to have everything installed automatically:

find if pybind11 is available in ./configure.ac. If so, set a variable in Makefile.conf
only if that variable is set, compile pycv during make. This is the same that we do for the python wrappers
again, only if that variable is set, install pycv during make install. This is what we do here for python wrappers. We have to decide where, it should be a place that is easily accessible.

Next, we will have to find out how to add a new conda package to install only pycv.

Iximiel commented 7 months ago

* you are using in many places `python3-config`. This should be customizable. Thinking about the possible use cases:
  * By default, use the variable `python_bin` configured in `Makefile.conf`. It's not trivial to access to Makefile variables from a shell script. My suggestion is to use a Makefile that includes `Makefile.conf` and then, if you need a shell script, pass the value of the relevant Makefile variables as arguments to the shell script or as environment variables. You can also get some idea from what we do [here](https://github.com/plumed/plumed2/blob/5cbb885d7c389749059285cea82faf483ff89daf/python/Makefile#L15-L22).

It is possible to grep the python bin from plumed config, but the problem is that the 'official way' of getting the flags is calling python3-config and the hard thing is to get the right one, I found something about that.

  * To allow a user to just compile this plugin _after_ plumed has been installed already somewhere, I guess you should allow the user to pass the name of the python interpreter likely as an environment variable. The variable could be `PYTHON_BIN`, consistently with what we use for `./configure`

In that case I would use the standaloneCompile.sh, and fish the pythonbin from plumed config or if not found use the one that is in the active venv. I think this is the less effort solution (for the user).

* `--no-as-needed` is not available on MacOS. I guess it's not needed.

On some system (for example on the default WSL, that should behave like ubuntu!!!) gcc -shared refuses to link libpython*.so to PythonCVInterface.so, I do not undestand why, since from manual --no-as-needed restores the "default behaviour", and it should be needed, since no --as-needed has been stated before, and there are calls for the Python lib symbols in ActionWithPython, and that should make the libpython to be linked even with --as-needed. (uses gcc 11.4.0). I am confused by the CI passing that should be also using Ubuntu...

On mac you get an error? Or just a warning?

Iximiel commented 7 months ago

Before merging your fixes, I'd prefer to keep for a few time the 'standard make' since is more agile for doing quick fixes.

What I did here

#assign a default value to python bin and plumed
python_bin=${python_bin:-python}
plumed_program_name=${plumed_program_name:-plumed}
#python3-config may be any python3.* version, this should avoid contamination from other environments
#and use pythonX.Y-config, as suggested in the manual
pyver=$($python_bin -c "import sysconfig;print(sysconfig.get_config_var('VERSION'))")
python_config=python${pyver}-config

Is to make more robust the use of python-config, since there may be more python 3. version in the same system, but python3-config will cover only one

Iximiel commented 7 months ago

@GiovanniBussi

find if pybind11 is available in ./configure.ac. If so, set a variable in Makefile.conf

Should I do something like this? (with autoconf2.69)

GiovanniBussi commented 6 months ago

@Iximiel sorry for the delay. I was just thinking at checking if pybind11 is available in the same way as we check if cython and setuptools are available. In this way you can leave the logic of setting the linking flags in a single place.

Iximiel commented 6 months ago

@Iximiel sorry for the delay. I was just thinking at checking if pybind11 is available in the same way as we check if cython and setuptools are available. In this way you can leave the logic of setting the linking flags in a single place.

So like this? (with autoconf2.69)? I added a few lines to the Makefile.conf, so that if at time of configure there is pybind11 in the python environment you get the ld and compile flags pre-baked there, if these are not present the sh will try to fetch them from the environment.

I think this solution should be refined, but I need some feedback to know in which direction: Because as now, you can pycv (canPyCV=@PLUMED_CAN_PYCV@ and related definition) even if there is no cython, even if with no cython there is no PYTHON_BIN set.

GiovanniBussi commented 6 months ago

I think this is unnecessarily complicated and forces you to do the work twice. To me the simplest logic would be:

at configure time:
- check if PYTHON_BIN has been set, otherwise search for a python3 interpreter (without or without cython/pybind11, it does not matter).
- then, check if cython is installed. If so, set a Makefile variable
- then, check if pybind11 is installed. If so, set a Makefile variable
at make time:
- if cython was found, use it to build the wrappers
- if pybind11 was found, use it to build pycv (HERE YOU FIND THE FLAGS)
at make install time:
- if cython was found, install the wrappers
- if pybind11 was found, install pycv (somewhere, I think we didn't decide yet where)

Otherwise (this will be done in the conda builders):

for python wrappers:
- if the preprocessed plumed.c file is available use it (we provide it on pip so that the user does not need cython). otherwise use cython. Then use setuptools to build the wrappers
for pycv:
- search if plumed is available (you need to link against plumed here, which is not true for python wrappers)
- if so, search now for pybind11 (it might not have been available at that time)
- if available, use it to build pycv (HERE YOU FIND THE FLAGS)

Does it make sense?

Iximiel commented 6 months ago

I do not understand why I should not search the flags at configure time: if I do not set them there, why would I need to see if pybind11 is installed at configure time?

For the other things, I think is ok: I'll have to to change the "plumed can python" statement that now is the ifdef python_bin in python/Makefile to something more flexible and remove the PYTHON_BIN= from the "failed to find cython" branch in the ac, if you are ok with this

GiovanniBussi commented 6 months ago

I do not understand why I should not search the flags at configure time: if I do not set them there, why would I need to see if pybind11 is installed at configure time?

Becausing searching the flags require some non trivial logic (e.g. to find the name of the proper python-config executable, as far as I understood). You do not want to implement this logic both in configure and in pycv build script. Since you need this logic in pycv build script (for people that want to build pycv on top of a preinstalled plumed), I would do it only there. Does it make sense? Or did I miss anything?

I'll have to to change the "plumed can python" statement that now is the ifdef python_bin in python/Makefile to something more flexible and remove the PYTHON_BIN= from the "failed to find cython" branch in the ac, if you are ok with this

Yes. I guess there should be Makefile variables such as:

python_bin (name)
plumed_python_has_cython (yes/no), can only be yes ispython_bin` is not empty.
plumed_python_has_pybind11 (yes/no), can only be yes ispython_bin` is not empty.

or similar

GiovanniBussi commented 6 months ago

why would I need to see if pybind11 is installed at configure time Regarding this, because you need to know if you should build the pycv module or not.

I don't think this is the only possible way, but it is consistent with what we do in the python wrappers: we find in configure if cython is available or not.

Iximiel commented 6 months ago

Becausing searching the flags require some non trivial logic (e.g. to find the name of the proper python-config executable, as far as I understood). You do not want to implement this logic both in configure and in pycv build script. Since you need this logic in pycv build script (for people that want to build pycv on top of a preinstalled plumed), I would do it only there. Does it make sense? Or did I miss anything?

If you see the branch I linked, I think I found a simple workaround. the cflags for pybind (it's header-only so no need for ld flags) can be found simply with the python executable (-m pybind11 --includes)

Yes. I guess there should be Makefile variables such as:

* `python_bin` (name)

* `plumed_python_has_cython` (`yes/no`), can only be `yes is `python_bin` is not empty.

* `plumed_python_has_pybind11` (`yes/no`), can only be `yes is `python_bin` is not empty.

or similar

the checks for cython/pybind cannot be done without python_bin set, from the original ac, so I think we can pass from assuming that if python_bin is set, then cython have been found, to "if confugure has found cython/pybind then python_bin is set", and of course keeping python_bin into the Makefile.conf I think that on this we are aligned

GiovanniBussi commented 6 months ago

If you see the branch I linked, I think I found a simple workaround. the cflags for pybind (it's header-only so no need for ld flags) can be found simply with the python executable (-m pybind11 --includes)

OK, so it's fine to do it twice if you prefer. But do not add these flags to general INCLUDE flags, because we do not want pybind11 to be used in random places (for now).

Iximiel commented 6 months ago

If you see the branch I linked, I think I found a simple workaround. the cflags for pybind (it's header-only so no need for ld flags) can be found simply with the python executable (-m pybind11 --includes)

OK, so it's fine to do it twice if you prefer. But do not add these flags to general INCLUDE flags, because we do not want pybind11 to be used in random places (for now).

as now is done like this:

canPyCV=@PLUMED_CAN_PYCV@
pybind11_cflags=@PYBIND11_CFLAGS@
python_cf_embedded=@PYTHON_CFLAGS@
python_ld_embedded=@PYTHON_LDFLAGS@

GiovanniBussi commented 6 months ago

it's header-only so no need for ld flags

If it can also be compiled with some #define that allows to bring it in a dedicated namespace (PLMD::pybind11) and licence allows it, we might even think of including the code as we do for asmjit. This namespacing stuff is necessary to avoid conflicts with MD codes using a different version of pybind11.

It might be worth if it's API is not very stable (and at some point we need to test for a specific pybind11 version). Otherwise, I think the current approach is perfect.

Iximiel commented 6 months ago

it's header-only so no need for ld flags

If it can also be compiled with some #define that allows to bring it in a dedicated namespace (PLMD::pybind11) and licence allows it, we might even think of including the code as we do for asmjit. This namespacing stuff is necessary to avoid conflicts with MD codes using a different version of pybind11.

I don't think it has many defines, but everything there is within their own pybind11 namespace and you do not have to fear about name conficts, because its (nearly) mandatory to compile with -fvisibility=hidden, exacltly for avoiding conflicts

It might be worth if it's API is not very stable (and at some point we need to test for a specific pybind11 version). Otherwise, I think the current approach is perfect.

I do not know if the API of pybind is instable, but thanks to pip we can freeze it with an ==x.y in the requirements

GiovanniBussi commented 6 months ago

I don't think it has many defines, but everything there is within their own pybind11 namespace and you do not have to fear about name conficts, because its (nearly) mandatory to compile with -fvisibility=hidden, exacltly for avoiding conflicts

Interesting

Iximiel commented 6 months ago

I finally merged the autoconf modification, now if the ingredients (numpy and pybind11) are already present at plumed configuration, they will be automatically used for compiling PyCV, otherwise is up to the user to set up the correct variables,

As now I did not rebased the branch onto the macOsS 12 commit

carlocamilloni commented 5 months ago

@Iximiel @GiovanniBussi could we merge this and release a beta version of plumed 2.10? so that additional changes can be merged in new pull request?

GiovanniBussi commented 5 months ago

If @Iximiel confirms I think we can merge. Conda build is failing for unrelated reasons

Iximiel commented 5 months ago

I am ok with the merge, the thing on the pbc can be addressed in an ad hoc PR

plumed / plumed2