NEP 29: new release schedule for scientific python packages

[jngrad]

The NEP 29 - Recommend Python and Numpy version support as a community policy standard proposes a new release schedule for numpy. Major python packages like matplotlib, jupyter and scipy have already adopted NEP 29, which means that most python dependencies of espresso will be affected either directly or indirectly. The proposed release cycle is 42 months, which is slightly shorter than the 48 months time window for espresso (we support the last two Ubuntu LTS).

This has consequences on our own release schedule. There is a handy support table and drop schedule to keep track of when to update our requirements.txt. We'll have to be more strict about version pinning for the few python packages that cannot be installed from Ubuntu repositories, to avoid e.g. #3420. We might have to drop support for Ubuntu 16.04 LTS in the planned espresso 4.1.3 release (due several months after Ubuntu 20.04 LTS is out), and tweak our docker .gitlab-ci.yml to test Ubuntu 16.04 images against the 4.1.2 tag instead of the 4.1 branch.

[RudolfWeeber]

The <https://numpy.org/neps/nep-0029-deprecation_policy.html> NEP 29 - Recommend Python and Numpy version support as a community policy standard proposes a new release schedule for numpy. Major python >packages like matplotlib, jupyter and scipy have already adopted NEP 29, which means that most python dependencies of espresso will be affected >either directly or indirectly. The proposed release cycle is 42 months, which is slightly shorter than the 48 months time window for espresso (we support the >last two Ubuntu LTS).

I think we don’t support 16.04 for any support schedule reasons. We have it because we are stuck with it for ICP internal reasons.

This has consequences on our own release schedule. There is a handy support table <https://numpy.org/neps/nep-0029-deprecation_policy.html#support-table> and drop schedule <https://numpy.org/neps/nep-0029-deprecation_policy.html#drop-schedule> to keep track of when to update our requirements.txt <https://github.com/espressomd/espresso/blob/python/requirements.txt> . >We'll have to be more strict about version pinning for the few python packages that cannot be installed from Ubuntu repositories, to avoid e.g. #3420 <#3420> . We >might have to drop support for Ubuntu 16.04 LTS in the planned espresso 4.1.3 release (due several months after Ubuntu 20.04 LTS is out), and tweak our >docker <https://github.com/espressomd/docker/blob/master/.gitlab-ci.yml> .gitlab-ci.yml to test Ubuntu 16.04 images against the 4.1.2 tag instead of the 4.1 branch.

Can’t we pin the pint version in the 16.04 container?

[jngrad]

The requirement MDAnalysis>=0.16 breaks for version 0.16 on Ubuntu 20.04. I'll increase the required version numbers once we're done with the ongoing simplification of CI images.

[jngrad]

Currently the centos:7 image still uses numpy 1.12.1. With numpy 1.13.0 and above, it looks like we no longer need the np.copy() trick in the testsuite to cast array_locked to mutable arrays inside np.testing.assert_array_equal().

[jngrad]

Actually we can only remove the np.copy from the first argument of np.testing.assert_* function, the second argument (the reference value) needs to be writeable, i.e. np.copy() is still required there. Although we'll have to remember that caveat, this still improves the readability of the tests by a lot. I was able to remove 140 superfluous np.copy() in the proof-of-concept bd193ff (100 np.copy() are still present). This requires installing numpy 1.14.0 in centos:7, which is the version we require in non-CentOS images.

@RudolfWeeber would you say the gain in readability is worth the trouble of remembering that the second argument has to be mutable? I would obviously take care of the merge conflicts in the waLBerla branch, and not touch the commented tests to avoid silent regressions.

[RudolfWeeber]

I don’t mind, either way.

[KaiSzuttor]

is np.copy such a big deal? I think we have more serious issues.

[fweik]

It's a surprising hurdle for writing tests, and as such we would be better off if it was not needed.

[KaiSzuttor]

I agree, but if we do not get rid of the calls completely I think it is not worth the effort.

[jngrad]

but if we do not get rid of the calls completely I think it is not worth the effort.

I wish we could remove np.copy() everywhere. Looking at the numpy code, the issue stems from the statement at numpy/numpy:numpy/testing/_private/utils.py#L769-L771. In flagged |= bool_array, bool_array is an array that inherits from our espressomd.utils.array_locked() class, and thus acquires the deleted __ior__() method. On the next flagged |= bool_array statement, flagged throws an exception. Not sure if there is a special member in python classes that could be called when a new object is instantiated from an existing object, to give the existing object an opportunity to return a writeable copy of itself if the type of the new instance is not array_locked (this would come into play when np.testing does bool_(array)).

[jngrad]

Looking at the exact block where we return a bool_(array_locked) object whose type inherits all our disabled special methods (line 750 in numpy/testing/_private/utils.py#L745-L750) gave me second thoughts. If the numpy project decides to change this if/else block in the future, there's a risk that e.g. the first argument needs to use np.copy() while the second argument doesn't. In that case, we would have to revert my changes.

For a long term solution, I looked at the numpy 1.13.0 new __array_ufunc__ special member callback, that allows you to intercept function calls (including basic matrix operations like multiply or add) and cast away the array_lock on-the-fly. This is what pint actually does to preserve SI units when passing their objects to numpy functions (pint/quantity.py#L1563-L1575). With this change, we could remove the need for np.copy() everywhere. I'll try to work out a solution. If this has unintended side-effects or too much overhead, we could instead inject subok=False for np.isnan() and np.isinf() to remove np.copy() in at least the testsuite.

Relevant numpy docs:
https://numpy.org/doc/stable/reference/arrays.classes.html
https://numpy.org/devdocs/user/basics.subclassing.html

[jngrad]

Update: we can't inject subok=False without also changing the type of the array_locked, otherwise we have infinite recursion. Changing the type by passing a view of the np.array works without noticeable overhead in the python tests. Once the CentOS 7 image is updated, 334 occurrences of np.copy(), np.array() and obj.copy() can be removed from the tests/samples/cython code. This will improve readability a lot and make CI a smoother experience.

[fweik]

@jngrad is there some immediate issue that needs fixing here? Is it worth putting much thought into this?

[jngrad]

is there some immediate issue that needs fixing here?

no

Is it worth putting much thought into this?

The fact that we have constructions in the code like np.array(np.copy(array_locked([i,j,k]))) - np.array([x,y,z]) suggests to me that our current array_locked class design goes against developer's expectations. The rightmost array casts the leftmost object into a np.array so the np.array(np.copy()) serves no purpose. We already require a numpy version that offers a stable framework for handling such cases gracefully for all function calls (3f5b195), instead of our currently hardcoded np.copy() special methods that only cover additions and subtractions. We could probably also convert all disabled special methods in array_locked by a set of strings that would be used to raise the exception in __array_ufunc__, as suggested in the numpy docs for ufunc-based class design.

addendum: besides the immediate effect of removing all occurrences of np.copy() and friends everywhere and not having to remember any side-effects in np.testing anymore, we can also do b = np.array([1.,1.,1.]); b *= system.part[0].pos without having to worry about the array_locked silently propagating its immutability to regular numpy arrays.

[fweik]

Just as a side note: The array_locked construction is a hack, which has come back multiple time to bite us. The underlying problem is the impedance mismatch between the reference semantics of python and our wonky interface code. Since we can't bend python to our will, the fundamental solution is to give our side also reference semantics. The reason for the whole array_locked thing is that our interface produces temporary values.

[christophlohrmann]

having to np.copy() values to check them is still an annoyance
https://gitlab.icp.uni-stuttgart.de/espressomd/espresso/-/jobs/257941

[jngrad]

The NEP 29 time table is now followed by ESPResSo, since we update dependency versions based on the Ubuntu LTS.

Regarding modernizing array_locked for better compatibility with numpy testing and user scripts (jngrad/espresso@3f5b195d) and removing all instances of np.copy() from the testsuite (jngrad/espresso@41e1c5b9), a majority of the core team opposed the change, and the testsuite has now diverged too much.

This ticket can be closed.