Add automated CI #1

jngrad · 2024-02-28T17:32:21Z

Description of changes:

add list of Python requirements
add a standard way to locate resource files in the project
write a minimal unit test and set up a CI pipeline

jngrad

Here is a minimal setup to run CI automatically. Feel free to add more tests in the new testsuite folder. It doesn't have to be too sophisticated. I would say the priority is to get this PR merged mid-March to get automated tests on any new contribution to avoid accidental regressions, and also get automatic security notifications from the list of Python packages (GitHub has a bot to scan requirements.txt against known security flaws). More thorough tests can be added at a later point in time, possibly in multiple independent PRs, together with a proper testing framework like pytest or unittest, as well as a documentation pipeline that would deploy the HTML files to a separate branch (instead of the main branch) that can be displayed in GitHub Pages.

testsuite/LYS_ASP_peptide.py

jngrad · 2024-02-28T17:40:04Z

testsuite/LYS_ASP_peptide.py

+
+# Create an instance of pyMBE library
+import pyMBE
+pmb = pyMBE.pymbe_library()


The logic with current_dir = os.path.dirname(...) was removed. The GitHub Action uses the $PYTHONPATH to properly detect pyMBE.

I think we need a better solution to ensure that the test runs both locally and in the Github CI

jngrad · 2024-02-28T17:47:22Z

testsuite/LYS_ASP_peptide.py

+# Load peptide parametrization from Lunkad, R. et al.  Molecular Systems Design & Engineering (2021), 6(2), 122-131.
+
+pmb.load_interaction_parameters (filename=pmb.get_resource('reference_parameters/interaction_parameters/Lunkad2021.txt'))
+pmb.load_pka_set (filename=pmb.get_resource('reference_parameters/pka_sets/CRC1991.txt'))


The function pmb.get_resource was introduced to facilitate locating files in the tree structure. In the long-term, you probably want to redesign the pyMBE project tree to something similar to this:

./ |---> pyMBE/ | |---> pyMBE.py | |---> __init__.py | |---> reference_parameters/ | |---> handy_script/ |---> testsuite/

This would help with packaging pyMBE on e.g. Pypi, and integrate it with tools like setuptools to avoid having to re-implement the same functionality in pmb.get_resource().

import importlib.resources import espressomd print(importlib.resources.path(espressomd, "version.py"))

Can you please move this to a dedicated issue to be considered for future developments?

jngrad · 2024-02-28T17:47:59Z

Makefile

 .PHONY: docs

 ESPResSo_build_path=~/software/espresso_v4.2/build/

 docs:
 	pdoc ./pyMBE.py -o ./docs --docformat google 

+testsuite:
+	${ESPResSo_build_path}/pypresso testsuite/LYS_ASP_peptide.py


The workflow uses the EESSI platform to serve pre-built scientific software packages. See the EESSI GitHub Action for more details.

Should we acknowledge this somehow? Maybe we could mention it in the README file

Please do! :)

Pointing to the EESSI website would be great: https://www.eessi.io

@boegel Sure :) I will add a note regarding the use of the EESSI platform in the README file in my next PR, including a link to the EESSI website

Acknowledgment added to #11. We can probably cite EESSI in the manuscript too. Thanks @boegel for designing this GitHub Action, it works really well!

You should thank @ocaisa here, not me :)

testsuite/LYS_ASP_peptide.py

pm-blanco · 2024-02-29T15:00:54Z

Thank you for taking care of this @jngrad. I would actually try to merge this to main within this week, so I can merge it to the branch were I am working on refactoring the tests for peptides 33-create-folder-paper.

I tried to run locally the current ci test testsuite/LYS_ASP_peptide.py but the path to pyMBE is now broken in the current implementation. I understand that the Github CI ads pyMBE to path when building the job, but it would be convenient to also be able to run the tests locally. What would be the best way to add pyMBE to path, also for the other sample scripts?

jngrad · 2024-02-29T15:49:37Z

I tried to run locally the current ci test testsuite/LYS_ASP_peptide.py but the path to pyMBE is now broken in the current implementation. I understand that the Github CI ads pyMBE to path when building the job, but it would be convenient to also be able to run the tests locally. What would be the best way to add pyMBE to path, also for the other sample scripts?

The original code used the following logic:

# Find path to pyMBE
current_dir= os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe())))
path_end_index=current_dir.find("pyMBE")
pyMBE_path=current_dir[0:path_end_index]+"pyMBE"
sys.path.insert(0, pyMBE_path)

It is quite fragile and imposes restrictions on the naming of folder in which the pyMBE repository gets cloned. Most projects use one of the following techniques:

a build system like CMake, which takes care of setting the paths via CTest
a symbolic link to the git repository in the ~/.local/lib/python3.10/site-packages folder
python virtual environments
module files

I think that pyMBE developers would find virtual environments to be the path of least resistance. Virtual environment will become common in Ubuntu 24.04. Here is how to configure a pyMBE-specific virtual environment:

# replace pypresso by python3 in the Makefile
sed -i 's/${ESPResSo_build_path}\/pypresso testsuite/python3 testsuite/' Makefile
# configure a pyMBE-specific virtual environment
python3 -m venv venv
echo 'export OLD_PYTHONPATH="${PYTHONPATH}"' >> venv/bin/activate
echo 'export PYTHONPATH="'$(realpath .)':'$(realpath ../espresso/build/src/python)'${PYTHONPATH:+:$PYTHONPATH}"' >> venv/bin/activate
sed -i '/deactivate () {/a export PYTHONPATH="${OLD_PYTHONPATH}"' venv/bin/activate
source venv/bin/activate
python3 -m pip install -r requirements.txt
python3 -m pip install scipy
deactivate

Notice how you no longer need to hardcode the path to the pypresso script in the Makefile. I don't think you actually need pypresso anywhere, python3 works just fine. Maybe you would need to double-check how run_test_protein.py exposes environment variables to jobs it starts, though. If you truly need pypresso, you could add extra echo and sed expressions to set and reset the $PATH variable.

Every time you want to use pyMBE, you need to update the current terminal environment variables by loading the python virtual environment:

$ echo $PYTHONPATH # empty on my workstation

$ source venv/bin/activate # enter the virtual environment
(venv) $ echo $PYTHONPATH
/work/jgrad/pybme:/work/jgrad/espresso/build/src/python
(venv) $ make testsuite
(venv) $ deactivate # leave the virtual environment
$ echo $PYTHONPATH  # original value should be restored

$

pm-blanco · 2024-02-29T16:13:26Z

@jngrad I followed your instructions and it works in my laptop, although I still need to run pypresso testsuite/LYS_ASP_peptide.py with pypresso. Otherwise I get:

python3 testsuite/LYS_ASP_peptide.py 
Traceback (most recent call last):
  File "/home/pablo/Escritorio/pyMBE/testsuite/LYS_ASP_peptide.py", line 8, in <module>
    import espressomd
  File "/home/pablo/Escritorio/espresso/build/src/python/espressomd/__init__.py", line 21, in <module>
    from . import _init
ImportError: libboost_serialization.so.1.71.0: cannot open shared object file: No such file or directory

Your proposed solution in a virtual environment sounds good to me, but I have some questions:

Would it work also when setting up pyMBE in a cluster? e.g. if the user has not sudo privileges.
If we do it like this, then we would need to provide some support for the user both for the setup and for activating pyMBE. One option that I can imagine that we could add those lines to the Makefile:

setup:
        python3 -m venv venv
        echo 'export OLD_PYTHONPATH="${PYTHONPATH}"' >> venv/bin/activate
        echo 'export PYTHONPATH="'$(realpath .)':'$(realpath ../espresso/build/src/python)'${PYTHONPATH:+:$PYTHONPATH}"' >> venv/bin/activate
        sed -i '/deactivate () {/a export PYTHONPATH="${OLD_PYTHONPATH}"' venv/bin/activate
source venv/bin/activate
        python3 -m pip install -r requirements.txt
        python3 -m pip install scipy
        deactivate
activate:
        source venv/bin/activate # enter the virtual environment

and document this both in the paper and the README file. Would this solution make sense to you?

jngrad · 2024-02-29T16:47:58Z

Your libboost_serialization.so.1.71.0 import error is really weird. The pypresso wrapper script doesn't touch $PATH nor $LD_LIBRARY_PATH. Do you modify $PATH in your .bashrc file? It could also be an issue with the virtual environment: on my workstation, it runs PATH="$VIRTUAL_ENV/bin:$PATH", which may insert the current working directory in the $PATH if $PATH was initially empty. Did you experience this exception on a Linux or macOS laptop? We are planning to write something about virtual environments in the ESPResSo user guide to help with the migration to Ubuntu 24.04, and the issue you are experiencing is something we definitively need to sort out.

I would not recommend putting the virtual environment setup in the Makefile, because that would re-introduce hardcoded paths again. There is no guarantee users put the espresso folder next to the pymbe folder. The Readme file and the user guide would be the right place, because it would force users to copy-paste the bash code and adapt it for their folder tree structure.

Virtual environments are part of the Python standard library since Python 3.3 and are meant to be used without sudo. They work by imitating a Linux installation with local /lib, /bin and /include folders that have precedence over the system-wide folders with the same name; this is quite similar to how modulefiles work, but they are a lot simpler because you can only load one environment (modulefiles load one environment per package, with expensive cross-compatibility checks). They work on compute clusters and with slurm.

pm-blanco · 2024-02-29T17:02:18Z

@jngrad I am working on Ubuntu 22.04.3 LTS. I did not modify $PATH in my .bashrc file, but it might have to do with some weird bad configuration of the virtual environment, I just now realized that is trying to execute some old espresso compilation that I had on my desktop ~/Escritorio/espresso/build/ instead than the one I actually use ~/espresso4.2/build/pypresso.

As a matter of fact, it looks that the problem that was experiencing has to do with your second point, this old compilation of espresso was indeed just next to my current pyMBE folder on my Desktop! How could we adapt this setup to an user-provided path to pypresso?

Could you please write the guide on how to setup and use pyMBE with this new logic of the virtual environment in the README file and later on the paper in section III-A?

jngrad · 2024-02-29T17:10:50Z

We could provide a script that users would call with pypresso to configure the environment, e.g.:

pypresso maintainer/setup_venv.py

This would be a lot easier for users who are not familiar with advanced shell syntax. But then it would fall on us to support the other shell interpreters (csh, fish, etc.), which I am not familiar with.

pm-blanco · 2024-02-29T17:18:03Z

@jngrad we can always provide a general description on how this should be setup and say that, in addition, for users with bash shell interpreters we provide a script maintainer/setup_venv.py to facilitate the setup. Then, we do not need to promise support for the other shell interpreters as we already provide the general guidelines on how to do it and we provide only a supporting tool for the most common case (users with bash shell script). I think that at least in the short run it would be an acceptable solution. What do you think?

pm-blanco · 2024-02-29T18:15:07Z

@jngrad anyway, we should probably disentangle this issue into two different PR though. Let's focus on the CI for now and we can open a separated PR for the setup and path issue and discuss it with the others. We can probably merge this PR as it is just moving pmb.get_resource to the right place in pyMBE.py (ordered alphabetically) so I can merge it with the refactoring of the tests and the repo structure that I am working on right now.

jngrad · 2024-02-29T18:43:05Z

I agree, let's avoid unnecessarily delaying this PR. You can run the testsuite locally with:

PYTHONPATH=$(realpath .) make testsuite

The virtual environment can be added in an independent PR.

pm-blanco · 2024-02-29T15:03:05Z

pyMBE.py

@@ -2749,3 +2749,17 @@ def write_output_vtf_file(self, espresso_system, filename):
            for particle in espresso_system.part:
                coordinates.write (f'{particle.id} \t {particle.pos[0]} \t {particle.pos[1]} \t {particle.pos[2]}\n')
        return 
+
+    def get_resource(self, path):


Currently, we enforce alphabetic order in the functions of the code to make it easier to sort them both in the code and in the API documentation

pm-blanco · 2024-02-29T15:05:06Z

testsuite/LYS_ASP_peptide.py

+
+# Create an instance of pyMBE library
+import pyMBE
+pmb = pyMBE.pymbe_library()


I think we need a better solution to ensure that the test runs both locally and in the Github CI

pm-blanco · 2024-02-29T15:09:02Z

testsuite/LYS_ASP_peptide.py

+# Load peptide parametrization from Lunkad, R. et al.  Molecular Systems Design & Engineering (2021), 6(2), 122-131.
+
+pmb.load_interaction_parameters (filename=pmb.get_resource('reference_parameters/interaction_parameters/Lunkad2021.txt'))
+pmb.load_pka_set (filename=pmb.get_resource('reference_parameters/pka_sets/CRC1991.txt'))


Can you please move this to a dedicated issue to be considered for future developments?

pm-blanco · 2024-02-29T15:17:19Z

testsuite/LYS_ASP_peptide.py

+
+# Load peptide parametrization from Lunkad, R. et al.  Molecular Systems Design & Engineering (2021), 6(2), 122-131.
+
+pmb.load_interaction_parameters (filename=pmb.get_resource('reference_parameters/interaction_parameters/Lunkad2021.txt'))


This logic of having to call pmb.get_resource(filename) to get the absolute path as an input to load of the parameters is rather cumbersome and probably not too intuitive for our users. Would not be better to have this dealt with internally by pyMBE? Maybe we could rename the input argument to local_path_to_file instead, what do you think?

We could have load_interaction_parameters() and friends internally call pmb.get_resource(). But then users wouldn't be able to read reference parameters that lie outside of the pyMBE root folder. This could be an issue if you plan to package pyMBE on PyPi, and could also be an issue in a teaching environment like the summer school, where Python packages are pinned before the school and only accessible in read-only mode.

I have no preference on how this is achieved. I opted for this temporary solution to remove the fragile code that existed before, and didn't manage to write a more elegant solution based on setuptools.pkg_resources or importlib.resources due to the lack of __init__.py file in the pyMBE tree structure.

pm-blanco · 2024-02-29T15:18:47Z

Makefile

 .PHONY: docs

 ESPResSo_build_path=~/software/espresso_v4.2/build/

 docs:
 	pdoc ./pyMBE.py -o ./docs --docformat google 

+testsuite:
+	${ESPResSo_build_path}/pypresso testsuite/LYS_ASP_peptide.py


Should we acknowledge this somehow? Maybe we could mention it in the README file

jngrad added 3 commits February 28, 2024 18:10

Python requirements

c35ae75

Prepare project for unit testing

caa1a64

GitHub Actions

6d54cd6

jngrad commented Feb 28, 2024

View reviewed changes

jngrad requested a review from pm-blanco February 28, 2024 18:04

pm-blanco assigned jngrad Feb 29, 2024

pm-blanco added the enhancement New feature or request label Feb 29, 2024

Move get_resource to preserve alphabetical order

893e566

pm-blanco approved these changes Feb 29, 2024

View reviewed changes

pm-blanco merged commit 8be6ca6 into main Feb 29, 2024
2 checks passed

pm-blanco deleted the github-action branch February 29, 2024 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add automated CI #1

Add automated CI #1

jngrad commented Feb 28, 2024

jngrad left a comment

jngrad Feb 28, 2024

pm-blanco Feb 29, 2024

jngrad Feb 28, 2024 •

edited

Loading

pm-blanco Feb 29, 2024

jngrad Feb 28, 2024

pm-blanco Feb 29, 2024

boegel Feb 29, 2024

pm-blanco Feb 29, 2024

jngrad Feb 29, 2024

boegel Mar 1, 2024

pm-blanco commented Feb 29, 2024

jngrad commented Feb 29, 2024

pm-blanco commented Feb 29, 2024 •

edited

Loading

jngrad commented Feb 29, 2024

pm-blanco commented Feb 29, 2024

jngrad commented Feb 29, 2024

pm-blanco commented Feb 29, 2024

pm-blanco commented Feb 29, 2024

jngrad commented Feb 29, 2024

pm-blanco Feb 29, 2024

pm-blanco Feb 29, 2024

pm-blanco Feb 29, 2024

pm-blanco Feb 29, 2024

jngrad Feb 29, 2024

pm-blanco Feb 29, 2024


		# Load peptide parametrization from Lunkad, R. et al. Molecular Systems Design & Engineering (2021), 6(2), 122-131.

		pmb.load_interaction_parameters (filename=pmb.get_resource('reference_parameters/interaction_parameters/Lunkad2021.txt'))

Add automated CI #1

Add automated CI #1

Conversation

jngrad commented Feb 28, 2024

jngrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jngrad Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm-blanco commented Feb 29, 2024

jngrad commented Feb 29, 2024

pm-blanco commented Feb 29, 2024 • edited Loading

jngrad commented Feb 29, 2024

pm-blanco commented Feb 29, 2024

jngrad commented Feb 29, 2024

pm-blanco commented Feb 29, 2024

pm-blanco commented Feb 29, 2024

jngrad commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jngrad Feb 28, 2024 •

edited

Loading

pm-blanco commented Feb 29, 2024 •

edited

Loading