Increasing pyMBE interoperability (part I)

[jorch28]

Summary

Application of the strategy pattern to delegate the responsability of executing the simulation engine to a particular engine class (e.g EspressoEngine).

Added

• EspressoEngine Class
• EspressoSystemProtocol Class to check that an the working engine corresponds to espresso.system instance
• reaction methods to EspressoEngine
• handy functions to EspressoEngine
• miscellaneous methods as (calculate_net_charge, change_volume_and_re-scale_particles,enable_motion_of_rigid_object,etc.)
• method add_instances_to_engine to add particles, bonds and angles to Espresso

Modified

• create_particle,create_residue, create_molecule,etc. to avoid an explicit dependence to Espresso
• unit_tests and functional_tests accordingly so that they introduce the particles, bonds and angles before choosing espresso as the working engine. Execute pmb methods from the simulation_engine when they depend on espresso.

Fixed

In commit: 217d15a

• class DiamondLattice. box_l=(self.mpc+1)*self.bond_l.magnitude / (np.sqrt(3)*0.25)
• method _create_hydrogel_chain. backbone vector=vec_between_nodes / (self.lattice_builder.mpc+1)

[jngrad]

Overall, I really like how the ESPResSo instance was made a data member of the pyMBE class. This really helps separate the pyMBE molecular building logic from the ESPResSo implementation details, and will make it easier for contributors from other communities to contribute to pyMBE, once LAMMPS is supported.

[jngrad]

I agree, this code looks strange. If the current behavior is intended, the loop is superfluous.

[pm-blanco]

I agree too, the loop should be removed and central_bead_pos should simply take the first value in list_of_first_residue_positions

[jorch28]

Then, do I modify it in this current pull request as Pablo is suggesting? Or do i set an Issue?

[jngrad]

Here and elsewhere, what is the intention behind the if/else chain? If the simulation engines share the same API, we could call self.simulation_engine.method() and let the class instance raise a NotImplementedError if the method doesn't exist for that engine, or a RuntimeError if the engine was never set, e.g. using a dummy engine class that throws for any method call. This would satisfy the open–closed principle: adding a new GROMACS simulation engine would only require implementing the corresponding class, and there would be no need to adapt all if/else chains in methods of pyMBE.py.

[jorch28]

Seems like a great idea to me!
To proceed with the modification:

1. By default the instance variable of the simulation_engine=DummySimulationengine() which overrides the methods from the BaseClass and raises a RuntimeError in case that the method is executed?
2. I will write the methods from the LammpsSimulationEngine with a statement that raises a NotImplementedError, so the implementation follows the open-closed principle

[jngrad]

Regarding point 1, here is the general pattern:

class MyClass:
    def test(self):
        return 5
    def __getattr__(self, attr):
        if attr not in self.__dict__:
            raise RuntimeError('You have not set up any simulation engine yet')
        return super().__getattr__(attr)


obj = MyClass()
obj.test() # doesn't raise
obj.method() # raises RuntimeError

I'm not sure how this interact with ABC metaclass options.

[jngrad]

Here and elsewhere in this notebook: that's a lot of text formatted for specific interpreters (ANSI escape code). Do we really need to ship this notebook with all outputs? There are ways to execute notebooks in CI to regenerate output cells, and to export the final notebook as a HTML page for the GitHub Pages.

[jorch28]

No, this has been done unintentionally. Now I'm going to delete the outputs.

[jngrad]

Here and in other new files: missing copyright notices.

[jngrad]

Here and elsewhere, there are numerous occurrences of _summary_. Are these todo notes?

[jorch28]

Nope. The extension AutoDocstring from VSCode generates the word summary automatically. I almost always keep it, because it is the header of the section as it happens as well with the words: Args, Return and sometimes Notes.

[pm-blanco]

I think that the place holder "_summary" in the docstrings should be replaced by a short description of what each method does.

[jngrad]

I'm a bit uneasy about pyMBE re-implementing a feature already provided by @warnings.deprecated(). I understand that it's only available in Python 3.13 onward, but Python 3.12 is old. It's no longer receiving bugfixes, only security fixes, and in about 9 months it won't be supported by new packages in the NumPy ecosystem (NEP 29 drop table). Ubuntu 24.04 LTS is the last OS that still imposes Python 3.12, and a couple of HPC clusters are stuck with Python 3.11 and 3.12.

We should discuss in the next meeting to which degree this is an issue for our users. Ubuntu 26.04 LTS is already available and the ICP plans to migrate to it in August. For HPC clusters where EESSI is available, Python 3.13 can be obtained from the 2025 stacks. On HPC clusters that don't provide EESSI, I build Python 3.14 from sources since older Python versions can be tricky to work with in conjunction with Cython and NumPy. Users of conda-like environments can install a new Python version with an extra command line option. I won't oppose this re-implementation, it's not a dealbreaker for me, but I think it's worth considering the maintenance aspects of re-implementing a functionality that is now widely available.

[jorch28]

I implemented this function so users now are warned that they have to move their usage of the function setup_langevin_dynamics, relax_espresso_system,setup_electrostatic_interactions , etc. To be employed by the simulation_engine as simulation_engine.setup_langevin_dynamics, simulation_engine.relax_espresso_system,etc. In order that when they start using the new release version not all of their old code breaks due to these changes being implemented.

Respect to upgrading pyMBE to python 3.13 we may find some troubles related to incompatibilities between the versions of the external python libraries, although maybe this is not a major issue, as the differences are between 3 minor versions .

[jngrad]

pyMBE meeting discussion: keep this re-implementation for now, since it will be a couple of months until our community fully migrates to the newer Python version, but replace it with @warnings.deprecated() from Python 3.13 before the next minor release of pyMBE.

[pm-blanco]

This comments should be removed

[pm-blanco]

remove this comment

[pm-blanco]

remove this comment

[pm-blanco]

time series should not be comitted to the main repository please remove this file and other similar ones

[jorch28]

@pm-blanco @jngrad
For version ESPREesSo 5.0-dev testsuite is failing due to an error importing espressomd. In the workflow execution logs it is shown that the module espresso is not loaded. I think this happens because in the push_pull.yaml the value for the key upload artifact in jobs.ubuntu.strategy.matrix.espresso.upload_artifact=False.

For ESPREsSO 4.2.2, all tests are passing, but they last longer than 300s and the last test 26/26 is not being executed. Did this happened before?

[jngrad]

There seems to be an issue with how module save handles the dev.eessi.io stack. In addition, the runner is executing actions with the wrong Node.js version. Let's drop the development stack in favor of the stable repository, and upgrade all software and operating systems. I'll open a separate PR, since it's orthogonal to this work.

[pm-blanco]

@jorch28 we have already updated the runner pipeline in the main pyMBE-dev repo, can you merge it here and see if now the tests pass?

[jorch28]

Now, for version 5.0.1 seems that there is no error related to import espressomd anymore. Thank you for your very fast answer! I'm going to add the method _check_bond_input to the LammpsSimulationEngine that is missing and therefore is not complying with the contract setup by the interface.
The actual problem was related to the differences between the methods and attributes from espressosystem in version 4.2.2 and version 5.0.1