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

Changing pycv into a proper python package #1134

Open
wants to merge 5 commits into
base: v2.10
Choose a base branch
from

Conversation

Iximiel
Copy link
Member

@Iximiel Iximiel commented Oct 14, 2024

Description

This should solve #1133, the TL;DR is "calling pycv from the python implementation gives problems"

I rewrote the building part of pyCV to look like a python package.

To install it you need plumed available in your path and you just need to use

pip install .

in the pycv dir.

To use it, it is slightly less straightforward (these solution do not work if you installed with pip install -e .):

calling plumed from the shell

When you install with pip you get an extra pycv executable in the bin of the environment. Calling it will print on the shell the path of the .so that contains the pycv actions.
You can copy-paste that path to your plumed.dat and you are ready to go.

calling plumed from python

If you are using plumed in a python script you'll need to have pycv available in the python environment. Then you can get the pycv object to load with:

from pycv import getPythonCVInterface
from plumed import Plumed
...
plmd = Plumed()
...
plmd.cmd("readInputLine",f"LOAD FILE={getPythonCVInterface()}")
...

I need to

  • refine this to compile 100% correctly (aka write a "findPLUMED.cmake")
  • find if there is a more robust way of passing the path
  • update the readme
  • rewrite its part of the CI (both for the plumed-that-calls-python tests and for the python-calling-plumed-that-calls-python tests)
  • update/add new tests
  • have some feedback on the user-friendliness
  • have some feedback on the names

A good number of important projects use scikit-build-core, among them LAMMPS and Cmake, so it is a project destined to remain.


This will unlikely pass the CI for the first few commits

@GiovanniBussi @tonigi, what do you think?

Target release

I would like my code to appear in release 2.10 or 2.11?

Type of contribution
  • 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
  • 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
  • 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.

plugins/pycv/pythontests/mypycv.py Fixed Show fixed Hide fixed
plugins/pycv/pythontests/test_run.py Fixed Show fixed Hide fixed
plugins/pycv/pythontests/test_run.py Fixed Show fixed Hide fixed
plugins/pycv/src/pycv/__init__.py Fixed Show fixed Hide fixed
@Iximiel Iximiel changed the title Changing pycv into a package Changing pycv into a proper python package Oct 14, 2024
@Iximiel
Copy link
Member Author

Iximiel commented Oct 23, 2024

It works, but I do not understand and I cannot reproduce locally the segmentation faults that are present in the fedora CI.

@Iximiel Iximiel marked this pull request as ready for review October 23, 2024 09:04
@carlocamilloni
Copy link
Member

@Iximiel @tonigi @GiovanniBussi is this ready to be merged?

@Iximiel
Copy link
Member Author

Iximiel commented Jan 16, 2025

no, @tonigi found some problems on mac, there is the mystery of fedora and the readme is updated to the previous version

@tonigi
Copy link
Contributor

tonigi commented Jan 22, 2025

Even though liked the "standalonecompile" idea, pip install seems to work better (e.g. in conda). I've pushed updates to the README. For what regards Mac, I put a note here https://github.com/tonigi/plumed2/blob/47e9b7128c73b3fa64d03cb66af8a2220c0dbe43/plugins/pycv/README.md?plain=1#L91 , which is hopefully sufficient to make it work.

@tonigi
Copy link
Contributor

tonigi commented Jan 22, 2025

In other words, ok for me to merge (after merging my changes to the readme).

@carlocamilloni
Copy link
Member

so i think that once the conflict and the readme are done i can merge it, correct @Iximiel ?

Now pycv works with pip

Reordering the installation procedure

added a sort of "FIND PLUMED" that does not work

setting up a second run option, adding an helper

removed the self-import gor getting the path of pycv

adding an example

Better CMakefiles

now it shoudl work with plumed from pkg-config or not installed (just needs the plumed executable in the path)

Restored the Makefile

restoring the standalone tests

adding some extra tests

updating the WF

Now pycv appears to load correcly

set up aslo a few basic tests for pyfunc

updated the docker recipes with the new installation method of pycv

using pip3 in the makefile

sourcing the correct bashrc in rocky8

Now the tests are compatible with  with GNUsed and BSDsed
@Iximiel Iximiel changed the base branch from master to v2.10 January 23, 2025 09:27
@tonigi
Copy link
Contributor

tonigi commented Jan 23, 2025

Something unrelated that would be nice, IMHO, could be to process the plugins with doxygen and add them to the manual (perhaps under a "Plugin" section).

@Iximiel
Copy link
Member Author

Iximiel commented Jan 23, 2025

I just squashed everything and rebased to v2.10.

I need to update the readme with the @tonigi comment and with an installation guide

And check if the CI on fedora had changed idea and now pass

@Iximiel
Copy link
Member Author

Iximiel commented Jan 23, 2025

Something unrelated that would be nice, IMHO, could be to process the plugins with doxygen and add them to the manual (perhaps under a "Plugin" section).

Yep, but that is for another PR

As now I am adding to the manual an example to create the documentation for the package with the built-in python tools (menaning with a discutible old-fashioned graphic style)

@Iximiel
Copy link
Member Author

Iximiel commented Jan 23, 2025

the fedora failures might be due to the python version being 3.12? Shall I comment the test temporarily under the rug @carlocamilloni @GiovanniBussi?

Looking at the logs i cann see that Rocky uses an ancient 3.6 (? o_O)

I forgot to specify that the extra tests I added, the ones that run with pytest, test pycv with the plumed python module

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.

3 participants