Cannot commit without Python2 installed

[sapetnioc]

When I try to commit with git in this project, I get the following error:

[INFO] Installing environment for https://gitlab.com/pycqa/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/python3', '-mvirtualenv', '/home/yann/.cache/pre-commit/repo56nkauy9/py_env-python2', '-p', 'python2')
return code: 1
expected return code: 0
stdout:
    RuntimeError: failed to find interpreter for Builtin discover of python_spec='python2'

stderr: (none)
Check the log at /home/yann/.cache/pre-commit/pre-commit.log

It works fine with option -n (disabling pre-commit). I do not have Python 2 installed on my system (Ubuntu 20.04), i believe this is the cause of the problem. But I do not know pre-commit enough to figure out how to solve this issue.

[DimitriPapadopoulos]

@ylep Is the Python 2 part still useful here? https://github.com/brainvisa/casa-distro/blob/41f6745b8071b7acc6a53b34dcd6629da6b51419/.pre-commit-config.yaml#L23-L26

[denisri]

No. Let's drop it.

[ylep]

I have to say I disagree: even though we are dropping support for Python 2 inside of the container, we want to keep support for the widest range of host machines, including those with only Python 2. Therefore, I’d really like to keep these Python 2 checks: they will at least prevent us developers from introducing Python3-specific syntax and unknowingly breaking Python 2 support...

The downside is that we require Python 2 to be installed on every developer’s machine, which sems like an acceptable compromise to me.

[sapetnioc]

I agree on the compatibility goal but I wonder if it would be possible to have the check done in tests rather than in pre-commit ? To my opinion, pre-commit is a bad experience for developers. If you make coding style mistakes you cannot go on and fix it later, even if the code is working. If one choose to ignore pre-commit with -n (it is bad but it is possible and tempting, therefore it happens), pre-commit tests are never done anymore. If we want to ensure coding style and Python2 compatibility in a branch, I believe tests are a better place than pre-commit.

By the way, casa-distro is not directly compatible with Python 2 only systems because bv uses a shebang with python3. Even users (not developers) on systems with only Python 2 will have an error and have to edit bv to run our software.

[denisri]

I don't have a strong opinion about all this. Having a few sanity check at commit time doesn't seem so bad to me, provided it is not a burden. Is the need for python2 installed a "burden" ? I'm not sure of it. And I also agree that it would be interesting to run such a test on the complete source code outside of precommit, too. As things are tested only at commit time, and only once in a file, it often happens that we cannot commit a file because previous commits were not passing precommit tests, which is not the responsibility of the current commit... Moreover, speaking of precommit I still wonder why I cannot run precommit any longer in the capsul project, and why I am apparently the only guy having this problem. Anyway I am forced to disable precommit at each commit (which is a little burden, too), and nothing gets tested at all. The error I have (on an Ubuntu 18.04 machine) is the following:

[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/home/dr144257/.cache/pre-commit/repoi2cfqg9t/py_env-python3/bin/python', '-mpip', 'install', '.')
return code: 1
expected return code: 0
stdout:
    Processing /home/dr144257/.cache/pre-commit/repoi2cfqg9t
      Preparing metadata (setup.py): started
      Preparing metadata (setup.py): finished with status 'error'

stderr:
      ERROR: Command errored out with exit status 1:
       command: /home/dr144257/.cache/pre-commit/repoi2cfqg9t/py_env-python3/bin/python -c 'import io, os, sys, setuptools, tokenize; sys.argv[0] = '"'"'/home/dr144257/.cache/pre-commit/repoi2cfqg9t/setup.py'"'"'; __file__='"'"'/home/dr144257/.cache/pre-commit/repoi2cfqg9t/setup.py'"'"';f = getattr(tokenize, '"'"'open'"'"', open)(__file__) if os.path.exists(__file__) else io.StringIO('"'"'from setuptools import setup; setup()'"'"');code = f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base /tmp/pip-pip-egg-info-2oj9x5kx
           cwd: /home/dr144257/.cache/pre-commit/repoi2cfqg9t/
      Complete output (6 lines):
      Traceback (most recent call last):
        File "<string>", line 1, in <module>
        File "/home/dr144257/.cache/pre-commit/repoi2cfqg9t/setup.py", line 1
          from __future__ import annotations
          ^
      SyntaxError: future feature annotations is not defined
      ----------------------------------------
    WARNING: Discarding file:///home/dr144257/.cache/pre-commit/repoi2cfqg9t. Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.
    ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.

Check the log at /home/dr144257/.cache/pre-commit/pre-commit.log

(I have erased .cache/pre-commit but it doesn't change anything). it seems to me that it is keeping trying to install a precommit version which is too recent for my python (3.6) and incompatible. However I don't understand why nobody else has this problem, and why I can still commit in other repositories.

Maybe this is another matter and I should open another ticket for it...

[DimitriPapadopoulos]

@denisri Is python pointing to Python 2 or Python 3 on your machine?

[denisri]

@denisri Is python pointing to Python 2 or Python 3 on your machine?

python2 (2.7.17)

[DimitriPapadopoulos]

Actually you do need Python >= 3.7:

• https://github.com/csuhan/ReDet/issues/14#issuecomment-814827012
• Can't import annotations from future

[denisri]

But ubuntu 18.04 comes with 2.7 and 3.6. We want to be able to, at least, commit changes, using the system python(s) ! Or at least is there a way to tell it "don't install an incompatible precommit, don't test for this version" (but as said before it would more or less negate the whole interest of precommit if some of us -me here- keep on commiting unchecked code). It's a bit crazy it can check python2.7 but not 3.6... and as said before it works in other projects so maybe it's something wrong in the precommit config in capsul ?

[denisri]

For capsul it was actually an (auto) upgrade in precommit config?. I have downgraded, which fixes the problem there. End of this digression...

[ylep]

I think we should have a collective discussion on pre-commit, but this is off-topic here, see https://github.com/brainvisa/brainvisa.github.io/issues/110

Going back to the original issue, do we agree that requiring python2 to be installed on developer machines is acceptable? (by the way, it is a non-issue if you commit inside the container, because the containers have python2). If so, we can reinstate the checks by merging https://github.com/brainvisa/casa-distro/pull/306.

[denisri]

I'm OK. @sapetnioc ...?

[sapetnioc]

Yes, no problem for me. Ok for a discussion about pre-commit. But I can live with pre-commit, we should be able to keep this discussion calm and not too long.