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

De-cythonize ESPResSo #4542

Open
jngrad opened this issue Jul 22, 2022 · 3 comments
Open

De-cythonize ESPResSo #4542

jngrad opened this issue Jul 22, 2022 · 3 comments

Comments

@jngrad
Copy link
Member

jngrad commented Jul 22, 2022

tl;dr: Most Cython files in ESPResSo can be rewritten as Python files using the ScriptInterface framework.

Background and problem statement

The Python interface of ESPResSo was originally designed using Cython, which could access C++ header files from the core and generate all the necessary glue code to connect the C++ shared objects to the Python interpreter. Cython is a convenient solution, and its syntax is easy to understand to contributors who have some Python experience but no C++ experience.

However, this design decision comes with several drawbacks that are not immediately visible to occasional contributors:

  • C++ files automatically generated from Cython files are several thousand lines long
    • they are tedious to debug in GDB
    • they take a long time to compile in CI
    • they trigger many compiler warnings and diagnostics due to how Cython v0.29 sometimes rely on sloppy (yet harmless) C++ constructs, e.g. unused variables, undocumented switch fallthroughs, etc.
    • exceptions in Cython code don't produce a useful traceback: only the last frame is shown (as a filename and line number)
  • removing global variables from the core is difficult, since they are used in the Cython code
  • Cython classes only run on the head node, which in turn requires introducing MPI callbacks in the core
  • Cython classes cannot easily "own" a resource (like an electrostatic actor)
    • the ownership information is necessary to control the lifetime of an object
    • storing a smart pointer to the owned resource in the Cython class requires a lot of knowledge about the internal mechanism for Cython/Python object construction, and makes pickling in a checkpoint file quite difficult

Proposed solution

The ScriptInterface framework was introduced to automatically generate the glue code between a Python class and a C++ class that holds a shared pointer to the owned resource. Although it requires writing a C++ class, there are many benefits:

  • clear separation of concerns in the core, with code split between public header files and private source files
  • no need for MPI callbacks anymore: the C++ interface class is already running in parallel, which mean standard boost::mpi code can be used instead of our custom MpiCallbacks framework
  • no need to use utils.check_type_or_throw_except() or utils.is_valid_type() anymore: this information is already encoded in the C++ class when using e.g. auto const value = get_value<Utils::Vector3d>(value), and a clear error message is generated automatically if the conversion failed
  • uniform interface for class methods: all methods use **kwargs, which reduces the risk of introducing silent API changes
  • no need to write __getstate__() and __setstate__() methods to checkpoint Python objects: they are generated automatically

In principle, only script_interface.pyx and script_interface.pxd are necessary, and those can probably be replaced by any other Cython-like interface library.

Curent situation

In release 4.2.0, many Cython source files were converted to Python using the ScriptInterface framework:

espresso/NEWS

Lines 766 to 771 in 18d49bf

* The `ScriptInterface` framework is now the preferred way to implement
new features. Existing features were converted to `ScriptInterface`
objects: bonded interactions (#4350), bond breakage (#4464), collision
detection (#4484), reaction methods (#4451), MPI-IO (#4455), H5MD (#4480),
cell system (#4511), actors, scafacos, electrostatics and magnetostatics
(#4506). The corresponding Cython files were converted to Python files.

This allowed for a significant reduction in the complexity of the MpiCallbacks framework:

espresso/NEWS

Lines 747 to 748 in 18d49bf

* The custom `MpiCallbacks` framework is being progressively replaced by
`boost::mpi` communication (#4506, #4511).

Next steps

The following components remain to be converted from Cython to Python:

@jngrad
Copy link
Member Author

jngrad commented Jul 26, 2022

Once the migration is complete, we should no longer be dependent on Cython 0.29. This would give us the opportunity to replace it with Cython 3.0, which breaks API in multiple ways (source), in particular: bytestrings are now unicode strings, cimport numpy now has side-effects, cdef class classes no longer allow methods starting with two underscores to be overridden in derived classes with methods of the same name, cdef class classes have extra arithmetic private methods. The Fedora project has a tracking ticket to monitor packages that break under Cython 3.0 in a CI environment (bugzilla 1823181). In 2020, 152 packages failed to build, in some cases due to changes in the GIL lock mechanism (2020 report).

kodiakhq bot added a commit that referenced this issue Apr 24, 2023
Fixes #4712

Description of changes:
- rewrite integrator as Python (partial fix for #4542)
- remove two MPI callbacks (partial fix for #4614)
- remove a global variable (partial fix for #2628)
@jngrad
Copy link
Member Author

jngrad commented Apr 26, 2023

Cython is deprecating conditional compilation. This is a feature we are still using in the remaining .pyx files.

When building ESPResSo with Cython version 3.0.0a11, the terminal is flooded with the following message:

warning: espressomd/myconfig.pxi:39:0: The 'DEF' statement is deprecated and will be removed in a
future Cython version. Consider using global variables, constants, and in-place literals instead.
See https://github.com/cython/cython/issues/4310

@jngrad
Copy link
Member Author

jngrad commented May 4, 2023

You can try out Cython 3.0 in a container or python environment with:

python3 -m pip install --user "Cython==3.0.0b2"

It is not possible to build ESPResSo. Conditional compilation is deprecated and leads to fatal errors. The nogil except + signature decorator syntax is only valid in Cython < 3.0 and the except + nogil syntax is only valid in Cython >= 3.0.

kodiakhq bot added a commit that referenced this issue May 5, 2023
Description of changes:
- bump requirements: CMake >= 3.20
- block CMake at configuration stage when Cython >= 3.0 is found
   - Cython 3.0 is not supported in ESPResSo, more details in #4542 (comment)
- add an extra guard for a known Doxygen bug
- use a ScaFaCoS library built for x86_64 architectures
- fix implicit type conversions in Lees-Edwards and use a named constant for invalid or uninitialized axes
kodiakhq bot added a commit that referenced this issue Jan 19, 2024
Description of changes:
- encapsulate thermostats and remove their Cython interface
   - use one C++ `ScriptInterface` class per core class (fixes #3980)
   - introduce the `ThermostatFlags` enum, allow deactivating a single thermostat, add an `is_active` flag accessible from the Python interface (fixes #4266)
- remove 20 MPI callbacks
   - partial fix for #4614
- remove thermostat and thermalized/rigid bond global variables
   - partial fix for #2628
- remove a significant amount of historical Cython code that relied on deprecated Cython features
   - partial fix for #4542
   - ESPResSo is now compatible with Cython 3.0
   - the `nogil` deprecation warning subsists
- improve testing of the thermostats interface
- API changes:
   - thermalized bond parameter `seed` was moved to `system.thermostat.set_thermalized_bond()`
      - this change better reflects the fact there is only one global seed for all thermalized bonds
      - until now this global seed was overwritten by any newly created thermalized bond, whether it was added to the system or not
   - LB thermostat `seed` parameter now gets written to the RNG seed (silent change)
      - until now, the RNG seed was hardcoded as `0` and the `seed` value was used to set the RNG counter, thus two independent simulations with a different seed would actually get the same particle coupling random noise sequence with a time lag equal to the difference between the two seeds
      -  this is a bugfix for ensemble runs
      - fixes #4848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant