Adopting a common branch

[rhfogh]

Unfortunately I missed the August meeting where this was on the agenda, but it would be good if we could discuss how we are planning to move towards using a common branch at the different beam lines. There are separate discussions for mxcubecore and mxcubeqt.

I do have a selfish interest: I am trying to integrate the GPhL workflow at different beamlines, and currently I have to code independently in four completely different branches: ALBA, EMBL-Hamburg, ESRF, and SOLEIL. Having one branch that many/all beamlines were at least close to would be a huge help. Of course upgrading production code is a lot of work, but it would be useful to agree on what we are aiming at and how current plans look.

mxcubecore/develop is the official leading branch. It is shared between all of us, and is being regularly updated, mainly by work at the ESRF. How far would people be from adopting it, and are there things we need to sort out? What are people's plans? Could we agree on versions for dependencies - which version of Python to use, before anything else?

[oldfielj-ansto]

When it comes to which Python versions to support, my suggestion would be to stick to the list of versions the PSF still supports.
Currently that would be Python 3.7-3.10 and soon 3.11, we can specify in the "requirements.txt" file which versions of packages should be installed for which versions of Python.

e.g.

matplotlib==2.2.5 ; python_version == '2.7'
matplotlib==3.3.4 ; python_version == '3.6'
matplotlib==3.5.1 ; python_version >= '3.7'

We can set things up so the GitHub action that executes PyTest runs consecutively for each supported Python version, I've been given access to try out GitHub actions on our public organisation and was planning on putting together a demo for a couple of other items in the codecamps next week, so I'll add this one to the list.

Australian Synchrotron's standard at the moment for active projects is to require Python 3.9+ compatibility, this will likely change to Python 3.10 when Python 3.11 is released next month. For the most part we've been running MXCuBE core without issue on Python 3.9 and earlier this year I ran some tests with Python 3.10 and saw no issues there.

[pkeller]

For background: Python >= 3.10 has a particular change that bit DIALS recently, specifically using __index__() instead of __int__() for implicit conversion to int. This is a breaking change for any code that relies on the previous behaviour of course. DIALS are unlucky in that the problematic code was in the graphical toolkit that they use and not in their own code, which has forced them to drop back to Python 3.9 for the foreseeable future.

Maybe an issue that needs further investigation/review?

[oldfielj-ansto]

@rhfogh Essentially this feature acts as a kind of dependency validation, if we were to specify a package in the "requirements.txt" file like so matplotlib==3.5.1 ; python_version >= '3.7', package managers such as PIP would evaluate this statement and validate that the active Python version was a minimum of Python 3.7, if the statement evaluates as False due to the user having a lower version than is required, the package manager will skip that line and move on to the next.

What this allows us to do is install the latest compatible version of a package that is supported by the current active Python environment.

https://peps.python.org/pep-0508/

[rhfogh]

OK, if that is what you mean by 'supporting Python versions 3.7-3.11' that sounds fine to me. Just as long as it is clear that the actual Python code must work under Python 3.7, not just under a newer version.

[oldfielj-ansto]

Yes, sorry I could have mad that clearer, I'd support ensuring the codebase is functional for the lowest supported Python version.

Since the latest Python version is largely backwards compatible with code from Python 3.6 onward it shouldn't be too much on an issue to maintain code using standard packages, third-party packages may be more problematic as between major versions they may deprecate a feature that is required by MXCuBE core necessitating a code change.

If a problem does pop up with one of the third-party packages it is relatively straight forward to account for this in the code.

import platform

if platform.python_version() < "3.8":
    # Run pre Python 3.8 specific code
    pass
else:
    # Run code specific to Python 3.8 and above 
    pass

[rhfogh]

I think the basic problem was that you and MO understood what this was about - and I did not. But I'd say that this is now cleared to everybody's satisfaction.

[marcus-oscarsson]

I would suggest that each site makes a summary of what their current situation is when it comes to mxcube version, environment it runs in and plans for updating. I could make a form/sheet for each site to fill in that would give us an idea of where we currently are that we then can start to discuss during our next developers meeting (in October).

When it comes to python version, I agree with @oldfielj-ansto that it seems very reasonable to support the officially maintained/supported Python versions being 3.7-3.11. It would in my opinion require something very exceptional for excluding a officially maintained python version.

@pkeller: I think the issue they had is that when calling an underlying C library in this case wxWidgets certain arguments are not coerced from float to int (so the type of the argument will be wrong). I'm not completely certain though, maybe you have some more information. I don't think it will effect us (perhaps older Qt bindings, but I doubt that it would be an issue in Qt5/6)

@oldfielj-ansto, all: Ive revived our CI with GitHub Actions, so there is now a basic Build and test workflow . It tests against python 3.7-3.10. It was done very quickly and there is plenty of room for improvements, so we are happy to see what you guys are doing. Also, feel free to play with our GitHub Actions if it can help you to test things, Ill give you access (let me know if it does not work) :)

[rhfogh]

I very much agree with Marcus' idea of making a summary of version and environment.

After discussing the question of Python versions with @marcus-oscarsson in this morning's MXCuBE3 meeting, I believe we agreed on a clarification. As I understand it, we should have a policy of supporting the oldest officially supported Python release at any time - which is currently Python 3.7. This means that any common code checked in must run without problems under this version, with a suitable set of dependencies. Since Python releases are generally backwards compatible, that code should also run without problems under newer versions. If people want to use features that only work under newer versions, they can do so in site-specific code, or if they make sure (with some kind of 'if python_version < 3.9: statement) that the code will also work on older supported versions. Similarly people are free to user newer dependencies that are not available in the older versions, but they must make sure that the python code will work directly under the older version as well.

How does that sound?

[pkeller]

@marcus-oscarsson The issue as manifested in DIALS/wxWidgets is a bit more subtle: Python <= 3.9 uses __int__() for implicit coercion to ints: this means that you can use instances of any type that defines or inherits __int__() as, for example, subscripts to arrays. IIUC wxWidgets uses floats in some places, relying on __int__() for the coercion. Python 3.10 changed to use __index__() for the coercion instead. The float type doesn't have an __index__() method, so the code in wxWidgets that uses floats as array subscripts broke.

At the end of the day, if any MXCuBE code relies on __int__() for implicit conversion, the situation isn't too bad: a combination of testing and code review can sort things out. The real problem is if third-party code that you rely on has this problem. DIALS were very unlucky: the problem was in a third-party library that wasn't covered by their automated testing. I'm not sure if the same thing might affect MXCuBE, but IMHO it would be worth satisfying yourselves on this point before you reach the point that you need to support Python 3.10

[oldfielj-ansto]

Thanks @marcus-oscarsson, I had a quick look at your workflow implementation, seems to effectively accomplish the same thing as we've done, but we went down the route of implementing linting support with Pre-Commit and added a few other tools to keep our code tidy.

@rhfogh This seems like a reasonable approach to take.