Old Python version (3.8) in pre-commit hooks

[fabcor-maxiv]

As highlighted in https://github.com/mxcube/mxcubecore/pull/834#issuecomment-1891719681 the pre-commit hooks seem to rely on an old version of Python, which is not the same version that conda installs, so it makes things a bit more annoying than necessary.

We should probably address this.

I am not sure we need to have a version range for Python in the conda-environment-dev.yml file. Maybe it is better to choose a version (major minor, for example 3.10), and then use the same for pre-commit hooks and everywhere else it makes sense.

[rhfogh]

Before we change the python version specified, we need to make sure that all installations are OK with that version. Essentially whatever version is specified in conda-environment-dev.yml would count as a de facto minimum version for installations, no? I am happy to upgrade to a newer version but we do need to check first.

[marcus-oscarsson]

The reason for the version range is that not all institutes support/use the same version for python on their beamlines. We can do another round table of supported python versions on our next meeting to see where we stand today. We have so far been considering as @rhfogh pointed out using 3.8 as the minimum version.

I thought that pre-commit uses a virtual environment and the python version specified in .pre-commit-config.yaml and not what you have in your environment. Did I misunderstand that ? What was the problem you had with 3.8/3.10 ?

[fabcor-maxiv]

Before we change the python version specified, we need to make sure that all installations are OK with that version.

Main thing to do in this issue ticket is to change the version in .pre-commit-config.yaml. We probably do not need to change conda-environment-dev.yml. Changing the Python version for pre-commit has only impact for the formatting, linting, and things

What was the problem you had with 3.8/3.10 ?

Not me. See here.

I guess by default conda will almost always install Python 3.10.x (because of the version range restriction in conda-environment-dev.yml: python >=3.8,<3.11). So I think pre-commit config should match this python: python3.10 (instead of the current python: python3.8).

Right now, if you happen to have a Python 3.8 installed on your machine, then you will not notice the issue because pre-commit will use it. But if, for example, the only Python you have available is the one installed in the conda environment (and it is not Python 3.8) then pre-commit will complain.

[marcus-oscarsson]

I'm using Python 3.10 and the pre-commit hooks works fine. My understanding was that pre-commit installs and runs everything in a separate environment, it seems to be the case for me at least, but I have really not checked how it works under the hood. That is why I was interesting in knowing more precisely why it failed in this case.

[fabcor-maxiv]

I'm using Python 3.10 and the pre-commit hooks works fine.

Strange... I would guess that you have a Python 3.8 somewhere (likely outside of the conda environment), which is what I have. But if I disable this Python 3..8 then I get the same error as mentioned here.

My understanding was that pre-commit installs and runs everything in a separate environment

I would guess a standard Python virtual environment (as created by venv or more likely virtualenv, not conda). These need the Python interpreter beforehand. It is not possible to create a Python 3.8 virtual environment if there is no Python 3.8 interpreter available on the system. It is different than with conda.

[fabcor-maxiv]

@marcus-oscarsson @rhfogh

Is there objection to update the Python version in the pre-commit hooks?

I will open separate ticket to discuss whether or not it makes sense to keep the version range in conda-environment.yml, I guess there is something I do not understand about this.

[fabcor-maxiv]

@marcus-oscarsson wrote:

we need to know what the issue was.

This was when the issue was uncovered: https://github.com/mxcube/mxcubecore/pull/834#issuecomment-1891719681

@rhfogh wrote:

The point of the pre-commit hooks is to run tests, right? And the agreed minimum version of Python is 3.8, right? Then, surely, you should run the tests under Python 3.8, so that you catch it if someone is using code that does not work in Python 3.8?

Our pre-commit hooks, as they are configured now, do not run the tests per se, but they run formatting, linting, and some Poetry check. And, yes, thinking about it again, you are right, it is probably best to run this sort of checks against the lowest supported Python version [^*]. This way we can make sure that we are not using syntax that has been added in Python 3.9 or Python 3.10.

I closed the pull request and I close this issue ticket. But I might need to make a pull request for mxcubeweb, because the mxcubeweb pre-commit hooks use the highest supported Python version, so we have the risk of introducing newer syntax.

And of course, we still need to figure out a way to fix the issue @HilbertCorsair uncovered.

[^*]: I actually do run the checks against the lowest supported Python in my (personal) projects usually, not sure why I got confused here.

[fabcor-maxiv]

I meant to reject the issue ticket.

[marcus-oscarsson]

@fabcor-maxiv don't worry the python and dependency versions can get confusing for most of us I guess :), for MXCuBE WEB I think we can keep it Python 3.10 or perhaps to be really safe use 3.9.