Document python requirement for tox/mypy & remove basepython from conf

[juhoautio]

Brief summary of the change made

Pin python version for pre-commit hooks.

Most notably this pins python version for the mypy pre-commit hook so that it's aligned.

So this is the same as #2629, but for pre-commit.

Document that tox must be installed with Python 3.8+ for mypy checks to work. That concerns both tox -e mypy and the mypy pre-commit hook.

Maybe it's worth mentioning that when user runs for example tox -e pre-commit -- install, a virtual environment is created at .tox/pre-commit/lib/python3.10 (assuming that tox was installed with 3.10). Or if one runs tox -e mypy for the first time, another virtual environment is created at .tox/mypy/lib/python3.8 (assuming that tox installation is with 3.8). And then, the version that was installed in the relevant .tox subfolder will be used for the mypy checks from that point onwards, no matter what kind of virtual environment user has activated.

This PR includes reverting #2629.

Hopefully when tox 4 is released it will be possible to set basepython = python3.10, python3.9, python3.8 to enforce a working version for running mypy checks in a flexible enough manner. For now it's only possible to set basepython with a single version, which is too restrictive. On the other hand for now there is a requirement for contributors to install tox with a new enough Python version.

Manual testing

Tested the pre-commit hook manually by running:

.git/hooks/pre-commit

I had installed pre-commit hooks with Python 3.7, so it failed:

fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
black....................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

src/sqlfluff/core/cached_property.py:7: error: Library stubs not installed for "backports.cached_property" (or incompatible with Python 3.7)
src/sqlfluff/core/cached_property.py:7: note: Hint: "python3 -m pip install types-backports"
src/sqlfluff/core/cached_property.py:7: note: (or run "mypy --install-types" to install all missing stub packages)
src/sqlfluff/core/cached_property.py:7: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)

flake8...................................................................Passed
doc8.................................................(no files to check)Skipped
yamllint.............................................(no files to check)Skipped

After – success (I tested setting basepython = 3.10 in tox.ini):

fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
black....................................................................Passed
mypy.....................................................................Passed
flake8...................................................................Passed
doc8.................................................(no files to check)Skipped
yamllint.............................................(no files to check)Skipped

But didn't end up pinning basepython here – instead removing it also from tox -e mypy.

The pre-commit also succeeds after installing pre-commit hooks with tox again after reinstalled tox using Python 3.8.

Are there any other side effects of this change that we should be aware of?

No.

Pull Request checklist

• Please confirm you have completed any of the necessary steps below.

• Included test cases to demonstrate any code changes, which may be one or more of the following:

  • .yml rule test cases in test/fixtures/rules/std_rule_cases.
  • .sql/.yml parser test cases in test/fixtures/dialects (note YML files can be auto generated with tox -e generate-fixture-yml).
  • Full autofix test cases in test/fixtures/linter/autofix.
  • Other.

• Added appropriate documentation for the change.

• Created GitHub issues for any relevant followup/future enhancements if appropriate.

[codecov]

Codecov Report

Merging #2644 (7e95f11) into main (5f01cc5) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #2644   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          163       163           
  Lines        12025     12025           
=========================================
  Hits         12025     12025           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f01cc5...7e95f11. Read the comment docs.

[tunetheweb]

What happens for those (like me!) who don't have Python 3.10 installed?

[juhoautio]

If you don't have 3.10, you get an error from tox when trying to install the pre-commit, saying that Python 3.10 cannot be found.

The same requirement for python version was already added in #2629 for tox -e mypy.

As said, 3.10 is the version that is also used in the gh actions. It is a possible source of problems if using some other version, even though it seems that currently mypy passes for this repo also with Python 3.8 and 3.9. Especially with 3.7 it fails even in the current stage.

For example, I was able to easily install Python 3.10.2 with pyenv after upgrading my pyenv.

If that is too restrictive, I wonder if something like basepython=python{3.10,3.9,3.8} is supported by tox 🤔 (I haven't tried it).

But IMHO it would be easier to stick with one version for mypy checks and require that all checks happen with that. I don't know if incompatibility is possible across the versions from 3.8..3.10, but if yes it sounds better to enforce the same version for everybody to avoid issues like that.

[tunetheweb]

We support Python 3.7 - 3.10. It's true we only run some of the tests in all 4 versions, and the other tests in just the latest (3.10) but that's more a pragmatic choice of not wasting compute as those other tests should be reasonably python agnostic.

I'm not sure that we should have pinned #2629 to 3.10 to be honest. Yes it aligns with our test suite but does force devs to use 3.10. I don't know if that's a problem to be honest but while we support 3.7 - 3.10 we might WANT the ability to develop under old versions. Maybe we can get away with not running mypy when doing that (though don't honestly see the reason for being that restrictive), but not being able to use pre-commit seems kinda limiting.

Is there a good reason for being this restrictive? Both here and in mypy?

[tunetheweb]

Also I'm unable to install Python 3.10 on my Mac and I'd very much like to continue developing SQLFluff 😉:

% tox -e py310 --devenv .venv
py310 create: /Users/barry/sqlfluff/.venv
ERROR: InterpreterNotFound: python3.10
_____________________________________________________________ summary ______________________________________________________________
ERROR:  py310: InterpreterNotFound: python3.10

% virtualenv --python python3.10 .venv
RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.10'

[juhoautio]

The reason is that as CI doesn’t guarantee that mypy checks pass on any other version than 3.10, so it’s nice to protect devs from running into situation where mypy strangely fails because of python version incompatibility.

This may be speculative. I’m not sure if mypy is ever going to fail with 3.8 or 3.9 either in this repo.

The other reason is that I didn’t know if it’s possible to require a range of python versions. Maybe that could be tried and it could be an acceptable middle ground.

But still, developing the project with certain python version is ok and supported. It’s just the mypy check that is safest to always run with the same version. It would not be productive to eg. try to make mypy pass also for 3.7 and as such I don’t see much point in ensuring that it works with any ohter lower version either.

But all of this is of course assuming that contributors are able to install 3.10. If you can’t install it, may I ask why is that? Have you given pyenv a try? For me it worked fine, just had to upgrade pyenv first.

[tunetheweb]

The reason is that as CI doesn’t guarantee that mypy checks pass on any other version than 3.10, so it’s nice to protect devs from running into situation where mypy strangely fails because of python version incompatibility.

As I said I think this is more a practicality of not needing to run every test on every env and it being a bit of a waste of compute in most cases.

This may be speculative. I’m not sure if mypy is ever going to fail with 3.8 or 3.9 either in this repo.

Yeah I don't see it being that likely either. Which is why I don't see the need to restrict. I don't run full CI on my machine and accept that some tests may fail in CI (for example I don't run dbt tests as rarely touch code that affects those and am happy for CI to pick me up if I ever do inadvertently change that).

But still, developing the project with certain python version is ok and supported. It’s just the mypy check that is safest to always run with the same version. It would not be productive to eg. try to make mypy pass also for 3.7 and as such I don’t see much point in ensuring that it works with any ohter lower version either.

Why I agree we shouldn't do lots of work arounds to support the older versions (mypy is a dev tool after all and not needed to run SQLFluff), I similarly don't see the need to be so restrictive - unless it is causing us problems, in which case I'll be supportive of it then!

But all of this is of course assuming that contributors are able to install 3.10. If you can’t install it, may I ask why is that? Have you given pyenv a try? For me it worked fine, just had to upgrade pyenv first.

Ah looks like I don't have it installed at all in my machine! Been so long since I originally set up 3.8 I forgot VirtualEnv didn't take care of all that for you and you still needed to actually install Python 3.8.

So I've two reasons against this change:

1. You're forcing me to install Python 3.10 when I haven't needed to before. Will this become 3.11 when it comes out? What if I'm already on 3.11 will I have to downgrade?
2. You're stopping me being able to develop in 3.7 effectively if I need to (e.g. to debug a 3.7 specific issue).

And not seeing any compelling reasons (at least for now) for this change.

@sqlfluff/sqlfluff-maintainers anyone else have any views on this? Maybe I just need to get over myself and install 3.10.

[barrywhart]

I tend to like being somewhat flexible about local dev environments (because machine config and updates can be a difficult PITA) but restrictive about the CI environment. Heck, I don't even use tox, yet I am still (arguably, ha!) a productive contributor. I tend to agree with @tunetheweb: it's fine to set up your machine a little weird as long as you solve your own issues. If you do weird stuff and ask me to fix it, I may politely decline.

It sounds like the Python version issues we're trying to address have occurred with Python 3.7. Is there a way we can make these two checks somewhat flexible, i.e. >=3.8? At minimum, supporting two different local versions is nice, i.e. the two most recent versions.

[tunetheweb]

What issues have we seen with 3.7 out of interest?

[tunetheweb]

I think we should revert this, and the one on line 97 as discussed.

[juhoautio]

What issues have we seen with 3.7 out of interest?

With 3.7 mypy fails when run as part of tox -e as well as the pre-commit hook (installed with tox as documented). There's some output in this PR's description.

Maybe these particular errors could be even fixed. But IMHO it becomes painful to try to keep up, if mypy is not checked on CI with 3.7. And the mypy checks are here just to help catch typing errors. There can be new typing features that won't work with older python, right? If we want to benefit from those we should not even try to run mypy with older python. Please correct me if I'm mistaken with that assumption.

The reason is that as CI doesn’t guarantee that mypy checks pass on any other version than 3.10, so it’s nice to protect devs from running into situation where mypy strangely fails because of python version incompatibility.

As I said I think this is more a practicality of not needing to run every test on every env and it being a bit of a waste of compute in most cases.

No but I wasn't suggesting to run mypy with multiple versions. Just one version, which is the same on CI as well as on developer machines. No wasting compute there?

I’m not sure if mypy is ever going to fail with 3.8 or 3.9 either in this repo.
Yeah I don't see it being that likely either. Which is why I don't see the need to restrict.

As said in #2629 the tradeoff is then that developers are forced to install their tox command itself with a specific python version (3.8+) as mentioned in #2629 description. In my opinion that is a severe restriction. That kind of model won't scale if one is developing multiple projects and if each project makes a similar stand, that they require tox to be installed with some different version, eg. some project requires 3.7 and another one requires 3.8.

But something worth looking into: does tox allow user to configure their own choice of default python on project level? If I could set some conf of my own in sqlfluff that says that basepython should be 3.10, that could be documented in CONTRIBUTING.md saying that 3.8+ is required to develop this project.

Still, I would prefer not having to rely on documenting caveats. It's clear enough to see an error message that "you're missing python 3.10", so then one can install it. If you just get an error that mypy fails on some checks, it takes much more time to troubleshoot and realize that it's about the python version, not something else. I spent considerable amount of time digging into it and it's especially surprising that tox makes a copy of python (with which tox was installed) in isolated places for both mypy target and pre-commit hooks.

You're forcing me to install Python 3.10 when I haven't needed to before.

The project is already "forcing" one to install at least 3.8, though. Is 3.10 somehow much more to require? Or is it still not so common for people to have multiple python versions installed in parallel?

Will this become 3.11 when it comes out? What if I'm already on 3.11 will I have to downgrade?

Hmm.. "downgrade"? You should be able to install multiple python versions so that tox can find the one it needs in each case. For example pyenv makes that easy. Your system python version can still be set to any version that you prefer, so there's no need to "downgrade" anything.

You're stopping me being able to develop in 3.7 effectively if I need to (e.g. to debug a 3.7 specific issue).

Not at all. The project can be "developed" with any of the supported runtime python versions, and you will be able to debug 3.7 specific issues. It's just the type checks with mypy that are run with a specific version.

And not seeing any compelling reasons (at least for now) for this change.

How about the point that I mentioned about having to install tox with a specific version AND the need to add more complexity in the CONTRIBUTING doc, that can be also easy to miss when setting up?

Is there a way we can make these two checks somewhat flexible, i.e. >=3.8?

Not supported by tox yet, but it seems that it will be soon.

This tox issue describes exactly similar issue that we have here: tox-dev/tox#1330.

Based on comment tox-dev/tox#1330 (comment) tox 4 (no stable release yet) will support specifying multiple basepython versions and tox will then just pick up one that is found. That could be the best way to offer flexibility.

Possible conclusions:

• Pin mypy stuff to 3.10 as I'm proposing (or for example 3.8 if that's somehow easier), require contributors to have 3.10 available
• Pin mypy stuff to 3.8+ once tox 4 released & adopted
• Revert pinning basepython & document in CONTRIBUTING that tox needs to be installed with 3.8+
• Edit: this doesn't seem to be supported (Look into possibility of user conf for tox basepython, if it works, document that in CONTRIBUTING instead)

[tunetheweb]

OK so I agree 3.7 is not supported for development. I didn't realise that.

However 3.8 is supported for development, so don't think we need to force 3.10 on people that want to use the pre-commit hook. And btw I've no particular problem upgrading my Python, but don't think we should insist on that without good reason. Others might not want to go through a whole install just to contribute here and that's what I'm concerned about.

It seems we have a good reason for Python 3.7, but not for 3.8 nor 3.9. But tox doesn't have an easy way (currently) of saying >= 3.8. Hmmm... tricky.

Looking at our download stats, python 3.7 is still far and away the most popular version that SQLFluff is downloaded for, followed by 3.8 and then 3.9 and finally 3.10. Now I know you're only proposing the minimum requirement of 3.10 for developing SQLFluff and not for installing it, but still it's out of kilter for how the package is mostly being used which surely implies there's a lot of older python versions still out there? And we should try to keep somewhat aligned to that rather than being 3 versions ahead IMHO.

So to me the options are:

• Pin 3.10 for dev (or at least for those wanting to use the pre-commit/mypy) but that may be too high a version for some.
• Pin 3.8 for dev (or at least for those wanting to use the pre-commit/mypy) but that may be too low a version for some.
• Document this better. But manual effort and easily missed.
• Investigate fixing 3.7 so mypy works with it so we don't need to pin at all and can support the same supported versions for running sqlfluff, as for developing it. But that's a bit of wasted effort - do we really need to support 3.7 for development?

I dunno the right answer here, but I have concerns over doing the first option. I worry we're going to insist on installing the latest Python on each release just to contribute and would rather keep the barriers to contribution low. Of course, those on lower Python versions, can still contribute - just not using mypy or pre-commit, but those are tools we should encourage not discourage.

I'll throw it out to @sqlfluff/sqlfluff-maintainers again. If others think I'm overly worrying about the concerns of pinning to 3.10 then happy to go with that.

[juhoautio]

Investigate fixing 3.7 so mypy works with it so we don't need to pin at all

I think this can be a source of more problems because it seems that there is something different especially for mypy before Python 3.8, so there could be similar issues later even if the current issue(s) is fixed. That is again kind of wasted effort to fix. And also if mypy is not run on CI also with 3.7 then it can be confusing for developers that mypy is not working locally if it breaks again for 3.7 some day.

[alanmcruickshank]

From my viewpoint::

• I don't think investing time into getting mypy support for 3.7 is going to be a worthwhile effort.
• I agree that ideally specifying >=3.8 would probably be the ideal solution here, but if I read this thread correctly, that's not easy to do.
• Something that's relatively future proof would be ideal so that we don't need to explicitly allow new python versions as they're released (e.g. 3.11).
• Given this is developer focussed rather than user focussed, I think solutions which rely on documentation or better instruction in docs could be a really neat solution.
• I'm actually ok with pinning a default python version in the tox.ini, but as long as devs know how to change it (either to suit their local env - for people like @tunetheweb ) or explicitly to test in different python versions (to the point that @barrywhart ) brought up. We'll have to make sure the docs have a good way of communicating that and that the way it's implemented doesn't impact the workflow too badly.

[tunetheweb]

• I'm actually ok with pinning a default python version in the tox.ini, but as long as devs know how to change it (either to suit their local env - for people like @tunetheweb ) or explicitly to test in different python versions (to the point that @barrywhart ) brought up. We'll have to make sure the docs have a good way of communicating that and that the way it's implemented doesn't impact the workflow too badly.

Can I ask what you mean by "I'm actually ok with pinning a default python version"? Especially since you put part of that in italics. My understanding is using basepython is not so much setting a preferred version, but insisting that version is used. Which means people like me that don't have Python 3.10 installed (and again, I WILL install it after this discussion is resolve, but speaking for others that don't want to do that) can no longer use pre-commit if this PR is merged.

The only way for me to override is, as I understand it, is to revert this change locally, and then remember not to accidentally commit that reversion. That's just not workable.

[WittierDinosaur]

As a potential compromise here - we could Dockerise development, and use 3.10 as the Docker base image?

[juhoautio]

To me it seems best for now to not pin basepython. And just improve the CONTRIBUTING doc to mention that tox should be wiht 3.8+. It is probably possible to add a requirement for basepython to be 3.8+ when tox 4 is adopted.

[juhoautio]

I have pushed changes and totally revamped the PR description. Please have a look when you have time!

[tunetheweb]

Thanks for this. I agree with this direction.

I do have some suggestions to reign back some of the verbosity. But would also be happy to merge as is if you prefer.

[tunetheweb]

Not sure this is needed and reads a little confusing. I think the Then covers it.

Suggested change

	A virtual environment can then be created and activated by running (check the [requirements](#requirements) before running this):
	A virtual environment can then be created and activated by running:

[juhoautio]

I wanted to be extra sure. If someone just jumps to this section and happens to have tox installed previously, they might miss the requirement. Are you ok with keeping the pointer?

[tunetheweb]

Yup. OK keeping them all as you prefer.

[tunetheweb]

Same feedback here. I know there’s a chance people can miss the early point but personally still find it a little overkill.

Suggested change

	To test locally, SQLFluff uses `tox` (check the [requirements](#requirements)!). The test suite can be run via:
	To test locally, SQLFluff uses `tox`. The test suite can be run via:

[tunetheweb]

And here. Do think we’ve gone from too little guidance here to too much. But maybe you feel strongly about leaving some of these in place since I know you say you wasted a good bit of time on this?

Suggested change

	For development convenience we also provide a `.pre-commit-config.yaml` file to allow the user to install a selection of pre-commit hooks by running (check the [requirements](#requirements) before running this):
	For development convenience we also provide a `.pre-commit-config.yaml` file to allow the user to install a selection of pre-commit hooks by running:

[juhoautio]

Out of the cases discussed, this is maybe the most important to keep the pointer. Because this is so far away from the main tox stuff, I can easily imagine someone jumping here just for the sake of installing pre-commit tools and again if that someone already has tox installed, it's not obvious that it could be wrong even though tox -e pre-commit -- install seems to succeed.

[juhoautio]

To make things worse, if you already ran this before you realized you need to change your tox installation, it's not trivial to realize that you also need to wipe out the .tox/pre-commit by hand and run tox -e pre-commit -- install again. Better safe than sorry, it's a pity that we can't enforce these programmatically.

[tunetheweb]

Happy to merge this now. Will leave it open until tomorrow in case anyone else has any comments or suggestions.

@juhoautio thank you very much for working through all the twists on this one. I hope we've landed on a solution that will help users, while not hindering them at the same time! Roll on tox4!