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

Unable to Lint or Render the PEPs Using Older Python Versions #2402

Closed
ericsnowcurrently opened this issue Mar 9, 2022 · 17 comments
Closed

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Mar 9, 2022

Context: my /usr/bin/python3 is 3.6. I've also tried with 3.7 and 3.8. I've tried it with venvs and without.

There are a number of problems here:

  • Makefile hard-codes $(PYTHON) to /usr/bin/python3
  • there is no way to specify which python executable I want to use (e.g. passing the "PYTHON" variable to make)
  • make lint fails (assignment expressions)
  • make sphinx fails (no "from future import __annotations__", missing "from future" statements, no str.removeprefix, no __class_getitem__)
  • we are running pre-commit rather than `$(PYTHON) -m pre_commit
  • CI hides these problems

I would expect to be able to do all of the above using older Python versions. 3.8+ would be good, but 3.6+ would be better.

@ericsnowcurrently ericsnowcurrently changed the title Unable to Lint or Render the PEPs Using Older Python Versions Unable to Render the PEPs Using Older Python Versions Mar 9, 2022
@ericsnowcurrently ericsnowcurrently changed the title Unable to Render the PEPs Using Older Python Versions Unable to Lint or Render the PEPs Using Older Python Versions Mar 9, 2022
@AA-Turner
Copy link
Member

Rendering PEPs requires 3.9 or later, I can't speak to linting.

there is no way to specify which python executable I want to use

I thought using the $(PYTHON) syntax allows this? I don't use make at all personally, though, so very likely wrong.

A

@ericsnowcurrently
Copy link
Member Author

Rendering PEPs requires 3.9 or later, I can't speak to linting.

How necessary is this? That presents a degree of inconvenience and could be dealt with without much fuss (it took me ten minutes to get it running under 3.6, e.g. wrapping problematic annotations in string literals.

If it is necessary then how can I make /usr/bin/python3 point to a 3.9+ install?

@ericsnowcurrently
Copy link
Member Author

FWIW, the message I'm getting is that I shouldn't be running make sphinx locally. Is that actually true?

@AA-Turner
Copy link
Member

pre-commit itself requires 3.7 or later, having checked.

we are running pre-commit rather than $(PYTHON) -m pre_commit

It is my understanding that pre-commit sees itself as a command line tool that happens to be written in Python, rather than a Python tool, so their reccomended invocation is pre-commit blah. I see the issue though, and it isn't ideal.

A

@AA-Turner
Copy link
Member

FWIW, the message I'm getting is that I shouldn't be running make sphinx locally. Is that actually true?

No! I'm sorry if I gave this impression. I would happily take suggestions on how to make this better for unix users -- I just don't know what to do to best fix this.

A

@ericsnowcurrently
Copy link
Member Author

pre-commit itself requires 3.7 or later, having checked.

Hmm, it worked fine for me on 3.6 after I fixed the pep_sphinx_extensions code.

@AA-Turner
Copy link
Member

Hmm, it worked fine for me on 3.6

I wasgoing from pre-commit/pre-commit@04de6a2, though I don't doubt you.

A

@ericsnowcurrently
Copy link
Member Author

It is my understanding that pre-commit sees itself as a command line tool that happens to be written in Python, rather than a Python tool, so their reccomended invocation is pre-commit blah.

Their shbang is hard-coded to /usr/bin/python3, which is super problematic. If they want to be used as a command-line tool then they should not support python -m pre_commit. Incidentally, pre-commit is basically just an alias for /usr/bin/python3 -m pre_commit.

@AA-Turner
Copy link
Member

The broader point here is that our guidance for PEP authors is clearly not good enough, leading to #2402, #2403, #2404.

Firstly there's the (hopefully simple) fix of changing the python invocations in make &c so that it searches PATH. Secondly, we should make the minimum Python requirement more explicit (is it documented currently?).

Eric -- I'd appreciate any general comment as to pain points -- happy to take full responsibility for my where changes have caused you headaches.

A

@ericsnowcurrently
Copy link
Member Author

I would happily take suggestions on how to make this better for unix users -- I just don't know what to do to best fix this.

Thanks! My suggestions:

  • make the changes in the PEPs code to be runnable under some minimum version that matches what is still in common use
  • change Makefile to allow $(PYTHON) to be passed in via an env var, to override the default
  • run pre-commit using $(PYTHON) -m pre_commit
  • run make targets in a venv by default (Make targets don't use venv #2403)

happy to take full responsibility for my where changes have caused you headaches.

It's not all that bad. It hasn't caused me any real problems. Mostly I have ended up avoiding doing much with PEPs because of these things.

@ericsnowcurrently
Copy link
Member Author

Regardless, I appreciate your effort!

@CAM-Gerlach
Copy link
Member

Sorry for the delay in responding; I've been dealing with urgent family matters most of today.

I maintain the linting infra, so I can speak to that part. As it seems you've discussed, it ultimately depends on the Python versions supported by pre-commit, which should include any supported version (i.e. currently 3.7+). If you're using Python 3.6, which is end of life and no longer supported by the Python team (i.e. this organization), older versions of pre-commit should (IIRC) still support it—though that is not guaranteed and may change eventually.

Hmm, it worked fine for me on 3.6 after I fixed the pep_sphinx_extensions code.

Oh, I initially wasn't clear on the specific issue being reported, and thought you were describing an error in the linting system itself, rather than a check failure, and wrote a response accordingly. What I'm guessing this is referring to, however, is that check-ast does not pass due to the parsed AST of those files not being valid on the runtime Python version you're using. We could simply skip said directory for that check; otherwise, all the hooks should work with any supported (3.7+) Python version.

As for the make stuff, I'll admit my make-fu is not very strong, as my primary dev machine is a Windows box and I'm much more familiar with other cross-platform tools. The lint and spellcheck targets are included mostly as a convenience for existing make users; PRs to fix issues/improve usability are always appreciated. Like @AA-Turner , I assumed that defining PYTHON=python3 and using that everywhere would use the currently activated python. but I'm guessing that's not the case?

Finally, as to the rendering system requiring Python 3.9+, I personally think it would be reasonable to accept a PR extending support to 3.7+ (i.e. the supported versions of Python) and maintain it on a best-effort basis, but I'd ultimately defer to @AA-Turner 's judgement, as its volunteer creator and maintainer. 3.6 is a different story, though, since it is no longer supported by the core team (i.e. this org) and an increasing amount of the dep stack, so supporting it here would be an issue both in principle and in practice.

@hugovk
Copy link
Member

hugovk commented Mar 10, 2022

  • Makefile hard-codes $(PYTHON) to /usr/bin/python3
  • there is no way to specify which python executable I want to use (e.g. passing the "PYTHON" variable to make)

You can override it like this:

make PYTHON=python3.7 lint
make PYTHON=/Users/huvankem/.pyenv/shims/python3.8 lint
  • we are running pre-commit rather than `$(PYTHON) -m pre_commit

Good point, please see PR #2408.

@brettcannon
Copy link
Member

I don't think any effort should be made to support 3.6 since we as a project don't even support that version of Python anymore.

Whether to support versions that are no longer receiving bug fixes I leave to the folks managing the infrastructure code now. I will say that PEP authorship is niche enough that asking folks to install a modern version of Python isn't unreasonable (and if it is then we should fix our installation story).

@hugovk
Copy link
Member

hugovk commented Mar 31, 2022

I also agree we don't need to support EOL versions such as 3.6, and am fine with requiring 3.9+.

@CAM-Gerlach
Copy link
Member

FYI, #2484 fixes the lint-side failure with the Python AST not being considered valid under older Python versions by disabling the check

@AA-Turner
Copy link
Member

I'll close this now -- we currently test on Python 3.9 and newer, with no plans to remove support for Python 3.9 any time soon!

A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants