Skip to content

A couple mypy/ruff fixes #13001

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented May 28, 2025

Fixes #13000.
Supersedes #12989.

EDIT:

  • Bump ruff/mypy in pyproject,toml to match versions in ci-constraints-requirements.txt.
  • Option warn-unused-configs = true requires incremental = false according to the --warn-unused-configs documentation:

    This requires turning off incremental mode using --no-incremental.

@alex
Copy link
Member

alex commented May 28, 2025

We added this job less than a week ago to account for the fact that mypy on 3.8 does, in fact, fail in places where 3.13 doesn't. See 1be08b9. As a result, this PR regresses our CI.

Given this job is incredibly fast, I don't see a need to make changes here.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented May 28, 2025

I don't think it's "mypy on 3.8" failing, instead it's mypy 1.14 failing. What's the point of running mypy on Python 3.8 and running mypy 1.14 instead of 1.15? I mean, while Python 3.8 is a supported target platform of cryptography, what is the point of supporting an EOL version of Python for running static linting tools, given this has no impact on the runtime behaviour of cryptography under that version of Python?

Additionally, the purpose of this PR is not speed, the intent was to get rid of this CI warning:

pyproject.toml: [mypy]: Unrecognized option: strict_bytes = True

@alex
Copy link
Member

alex commented May 28, 2025

I don't believe that's correct. As the original issue describes, the issue that this test was added to catch reproduces on 3.9, which supports mypy 1.15. (You'll also see that the changelog for mypy 1.15 doesn't mention this feature: https://mypy-lang.blogspot.com/2025/02/mypy-115-released.html).

The point of this test is to verify that users will be able to typecheck their programs against cryptography.

AFAIK there's no way to resolve that warning without regressing this.

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the flake branch 2 times, most recently from b579a8c to 4a8e841 Compare May 28, 2025 12:01
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented May 28, 2025

I get it now. While there are no code path differences in cryptography itself, there may be differences in modules such as typing.

Now, if it's about Python 3.9, the way to resolve the warning is to run mypy with Python 3.9 and 3.13 instead of 3.8 and 3.13, as they all support 1.15.

But again, I don't see a fundamental reason to run mypy with Python 3.9 instead of a more recent version: it won't catch runtime issues of cryptography specific to that version of Python, will it? The practical reason is that developers may be stuck on a platform with older default versions of Python (I am myself stuck on an Ubuntu 20.04 server with Python 3.8 at times). I wonder whether --python-version can help here, it remains unclear to me from the documentation if it is designed to help with issues such as 1be08b9.

@alex
Copy link
Member

alex commented May 28, 2025

It's about supporting all the configurations we expect to work for our users.

I do not understand what your motivation is for this -- at this point it's consuming a lot of my time to explain this, and the upside is effectively nothing.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented May 28, 2025

It's about supporting all the configurations we expect to work for our users.

Are they expected to work for end-users of the cryptography module or developers/users that run mypy on the source code? That's the distinction I am making above.

@alex
Copy link
Member

alex commented May 28, 2025

Both.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented May 28, 2025

My point was that it's not useful to let developers run mypy under Python 3.8. However, after reading Issues with code at runtime, I realise there may be runtime issues after all:

Idiomatic use of type annotations can sometimes run up against what a given version of Python considers legal code.

I wonder whether these might be addressed using --python-version 3.8: is that option the same as running under Python 3.8, but without the CI warning?

Any way, let me read more about it and stop annoying you. I'll modify this PR to apply three very simple changes:

  • commit 4a8e841 for ruff,
  • turn off incremental mode as per the --warn-unused-configs documentation,
  • bump ruff and mpy in pyproject.toml to match ci-constraints-requirements.txt.

Option `warn_unused_configs` requires turning off incremental mode:
https://mypy.readthedocs.io/en/stable/config_file.html#confval-warn_unused_configs
No need to specify it explicitly, the same way we don't specifiy it with
mypy or sidst-check.
@DimitriPapadopoulos DimitriPapadopoulos changed the title Run flake tests with a single version of Python A couple mypy/ruff fixes May 28, 2025
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review May 28, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

No need to run flake test session with multiple Python versions?
2 participants