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

python version for CI formatting check #2681

Closed
jakkdl opened this issue Jul 3, 2023 · 11 comments · Fixed by #3049
Closed

python version for CI formatting check #2681

jakkdl opened this issue Jul 3, 2023 · 11 comments · Fixed by #3049

Comments

@jakkdl
Copy link
Member

jakkdl commented Jul 3, 2023

Is there any special reason we're staying on 3.8 for the CI tests? If we bumped it we would get additional typing features and not have to do from __future__ import annotations, and we're going to have to bump 3.8 eventually when it reaches EOL in 2024-10.
Does some tooling break if we were to typecheck with 3.10 or 3.11 features or something? Is there any best-practice wrt typechecking/linting your code on oldest/newest version supported?

@A5rocks
Copy link
Contributor

A5rocks commented Jul 4, 2023

I don't quite understand the issue here :P

Are you talking about just the formatting? Or the tests themselves. The tests themselves are due to our compatibility window. The formatting check shouldn't really care about the version:

  • from __future__ import annotations prevents runtime breakage in the tests
  • using unsupported type annotations means that 3rd party users cannot introspect our types at runtime on those other versions and I'm not quite sure if that's a price I'm personally willing to pay (though it's a small price, to be sure. It's probably fine).
  • mypy and black (probably flake8 too?) have configuration to change what version you are checking them on: they do not depend on the runtime version for more than syntax (at least for mypy, which is what I know)

However given all that: I think bumping the version for the formatting job would be fine, even if it doesn't change anything.

@jakkdl
Copy link
Member Author

jakkdl commented Jul 4, 2023

from __future__ import annotations prevents runtime breakage in the tests

oh right, derp. Sorry for the confusion

So unless the feature set of mypy/black/flake8 were to differ between installed versions (due to some new feature not getting implemented when running against an old AST, or similar), it shouldn't change anything in theory. I have a suspicion that stuff would actually behave differently in practice though, and we'd also get access to new language features in general (wrt stacktraces, speedups, etc). I think most tooling should have good 3.11 support now, but could go with 3.10 to be more conservative.

This behavior seemed to be due to the python version on the CI, though I'm not 100% if it's the python version it's run on vs the runtime flag of version to check against.

trio/trio/_channel.py

Lines 97 to 100 in 4816f0e

# written as a class so you can say open_memory_channel[int](5)
# Need to use Tuple instead of tuple due to CI check running on 3.8
class open_memory_channel(Tuple[MemorySendChannel[T], MemoryReceiveChannel[T]]):
def __new__( # type: ignore[misc] # "must return a subtype"

@A5rocks
Copy link
Contributor

A5rocks commented Jul 6, 2023

We probably didn't configure mypy to run against a certain python version. We do so for platforms (see check.sh) but nothing for python version as far as I can tell which means mypy will default to using the runtime version of Python. We didn't forget to do this for black, however.

@jakkdl
Copy link
Member Author

jakkdl commented Jul 6, 2023

Ah, you're right - and we should definitely set that so it's easier for developers to locally typecheck their code without having to run mypy inside a py38 virtualenv.
mypy.ini does have the lines

# TODO: run mypy against several OS/version combos in CI
# https://mypy.readthedocs.io/en/latest/command_line.html#platform-configuration

which suggests that somebody at some point wanted to address python versioning in CI.

@TeamSpen210
Copy link
Contributor

If we're not testing the version itself, it's probably better to use a newer one since then we'd get the advantages of all the recent speed improvements.

@jakkdl jakkdl changed the title python version for CI tests python version for CI formatting check Jul 28, 2023
@jakkdl
Copy link
Member Author

jakkdl commented Jul 28, 2023

Bumping the python version for the environment will modify *-requirements.txt - this is what happened in #2715, where the bot removed docs dependencies that are only required on python <3.11
That didn't break readthedocs, but test-requirements.txt will drop the tomli and the exceptiongroup dependency which I'm pretty sure will break tests running on 3.8. Could specify to always keep tomli, but not sure how to stop pip-compile from throwing out the conditional exceptiongroup.

Merely bumping it to 3.10 should work, or we could split the CI check in two (one on 3.8 running pip-compile, one on 3.11 doing the rest), or a more comprehensive change that also resolves dependabot breaking stuff and lets everything run on 3.11.

@A5rocks
Copy link
Contributor

A5rocks commented Jul 29, 2023

Good point. BTW I hate that dependabot security alerts are a thing. I'll just close them in the future as they are not worth the hassle.

I think we should find a pip-tools alternative that:
a) persists environment tags
b) (as part of the above, probably?) supports building the lockfile for all supported versions

I've seen things like pipgrip --lock but dunno if that works for us. (I remember it not working for my case, I can only find complaining about not having a way to add hashes, but there might have been more) It doesn't work. I can see there's demand for such a tool though.

@A5rocks
Copy link
Contributor

A5rocks commented Jul 29, 2023

For what it's worth, a friend has used https://gitlab.com/mitchhentges/pip-compile-cross-platform successfully for this exact problem. (Basically that just uses poetry to spit out a cross-platform lockfile for pip.......)

But that's not maintained much.

@jakkdl
Copy link
Member Author

jakkdl commented Jul 29, 2023

We could maybe just move to poetry or hatch?

And instead of having to close them, it must be possible to disable them for the repo, no? It's definitely silly when it's just about updates to the doc-requirements.

@A5rocks
Copy link
Contributor

A5rocks commented Jul 30, 2023

IIRC hatch doesn't do lockfiles. Poetry does, though I've got mixed feelings about Poetry as a whole. We do have a PR open about switching to it, though.

And it is possible to disable them for the repo but we'd need @njsmith to do so. I'll close them if they do show up though.

@jakkdl
Copy link
Member Author

jakkdl commented Jul 30, 2023

Yeah hatch doesn't do lockfiles. There's also PDM if you don't like Poetry, I have some experience with Poetry but don't have experience with either in a big project.

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

Successfully merging a pull request may close this issue.

3 participants