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

Run type-annotation linters in CI environment #2841

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

happz
Copy link
Collaborator

@happz happz commented Apr 10, 2024

The goal si to verify the code and its annotations are compatible with Python packages shipped as system packages.

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • adjust plugin docstring
  • modify the json schema
  • mention the version
  • include a release note

@happz happz added the ci | full test Pull request is ready for the full test execution label Apr 10, 2024
@happz happz force-pushed the tests-type-annotations-in-ci branch from c24cfd1 to 4278abb Compare April 18, 2024 09:15
@happz happz force-pushed the tests-type-annotations-in-ci branch from a7c67ac to 466b5c9 Compare May 6, 2024 12:12
@happz happz mentioned this pull request May 14, 2024
1 task
@happz happz force-pushed the tests-type-annotations-in-ci branch 2 times, most recently from ed705d9 to 11a2dcc Compare May 21, 2024 09:13
Comment on lines +22 to +33
rlRun "mypy_version=$(yq -r '.repos | .[] | select(.repo | test("^.*/mirrors-mypy$")) | .rev' ../../.pre-commit-config.yaml | tr -d 'v')"
rlRun "pyright_version=$(yq -r '.repos | .[] | select(.repo | test("^.*/pyright-python$")) | .rev' ../../.pre-commit-config.yaml | tr -d 'v')"

rlRun "rm -f requirements.txt && touch requirements.txt"
rlRun "echo 'mypy==v${mypy_version}' >> requirements.txt"
rlRun "echo 'pyright==v${pyright_version}' >> requirements.txt"
rlRun "yq -r '.repos | .[] | select(.repo | test(\"^.*/mirrors-mypy$\")) | .hooks[0].additional_dependencies | .[] | select(test(\"^types-.*\"))' ../../.pre-commit-config.yaml >> requirements.txt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just be using dnf install python3-mypy so that we are actually using the systems packages here? I see in the test logs that it is still downloading from pypi the packages.

Copy link
Contributor

@LecrisUT LecrisUT May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're only filtering types-* there. I was looking at urllib3-2.2.1 still being downloaded.

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
requests 2.28.2 requires urllib3<1.27,>=1.21.1, but you have urllib3 2.2.1 which is incompatible.

Looking more on the issue of urllib3, that seems to be quite a complicated can of worms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just be using dnf install python3-mypy so that we are actually using the systems packages here? I see in the test logs that it is still downloading from pypi the packages.

I wouldn't think so, because mypy (and pyright) are not tmt requirements, but linters. The idea is to use system dependencies, but defined linters looking at the project from outside. With system mypy, we might be using older versions often, which may not recognize issues newer versions were taught to report.

Oh, you're only filtering types-* there. I was looking at urllib3-2.2.1 still being downloaded.

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
requests 2.28.2 requires urllib3<1.27,>=1.21.1, but you have urllib3 2.2.1 which is incompatible.

Looking more on the issue of urllib3, that seems to be quite a complicated can of worms.

Yeah, it seems to be pulled in because of types-requests. TBH, I did not even check the log of pip install, and yeah, this is something I'd like to avoid. No idea why types-requests feels the need to pull in a genuine package, maybe because urllib3 does not ship type stubs separately, but an action like this would ruing the idea of "testing against system packages" :/

I think we'll get there, but it is indeed a bigger can of worms than it seemed last week :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't think so, because mypy (and pyright) are not tmt requirements, but linters. The idea is to use system dependencies, but defined linters looking at the project from outside. With system mypy, we might be using older versions often, which may not recognize issues newer versions were taught to report.

Yeah, fully agree. Was more concerned about the actual packages, but then I saw that you are only using types-*, which seems a-ok. But I think the version of the types-* needs to be matched with the version found in the system's pip, i.e. search the pre-commit-config.yaml (or maybe other approach later) for the types-*packages, search pip list or such for the actual package version, and then write the version constrain to match the actual package

No idea why types-requests feels the need to pull in a genuine package, maybe because urllib3 does not ship type stubs separately, but an action like this would ruing the idea of "testing against system packages" :/

Found out. It's because urllib3 >= 2.0 already includes typing, and the types-request constraint used, does not apply in F40 case, thus getting the latest version types-request which uses the latest urllib3 version. The workflow above should take care of that though, WDYT?

@LecrisUT
Copy link
Contributor

How about bumping some of the minimum requirements?

  • jinja2 >= 3.0: can get rid of types-jinja2
  • types-jsonschema might not be necessary anymore? Seems like the simple typing are merged (primary issue), and it's mainly quite advanced typing that we most likely will not use that is still open.
  • types-urllib3 can be removed once urllib3 >= 2.0 is landed. Epel9 might be tricky though.
  • types-babel can be removed from 2.12.1. Didn't find what the version is on Epel9, F39 can remove already
  • types-setuptools 🤷 what is this one used at?
  • types-docutils, types-Markdown, types-requests still needed for the foreseeable future.

But does mypy not have a more to automatically download the missing typings from typeshed?

@happz
Copy link
Collaborator Author

happz commented May 24, 2024

How about bumping some of the minimum requirements?

As long as they can be satisfied with RPMs available to us, I don't see a problem in general. I'm not sure what would happen if we bump a package version too much, rpmbuild would fail?

  • jinja2 >= 3.0: can get rid of types-jinja2
  • types-jsonschema might not be necessary anymore? Seems like the simple typing are merged (primary issue), and it's mainly quite advanced typing that we most likely will not use that is still open.
  • types-urllib3 can be removed once urllib3 >= 2.0 is landed. Epel9 might be tricky though.
  • types-babel can be removed from 2.12.1. Didn't find what the version is on Epel9, F39 can remove already
  • types-setuptools 🤷 what is this one used at?

Maybe something related to reousrce_files, importlib? I don't recall we'd use packaging-related functionality in tmt code for anything beyond that.

  • types-docutils, types-Markdown, types-requests still needed for the foreseeable future.

But does mypy not have a more to automatically download the missing typings from typeshed?

It does download missing types, with --install-types, on the other hand it seemed better to list them explicitly and make sure they are present in mypy and pyright environments rather than leave it on linter guessing.

@LecrisUT
Copy link
Contributor

LecrisUT commented May 24, 2024

  • types-setuptools 🤷 what is this one used at?

Maybe something related to reousrce_files, importlib? I don't recall we'd use packaging-related functionality in tmt code for anything beyond that.

types-setuptools seems to only cover pkg_resources and distutils, both of which tmt seems to have already upgraded. That's why I was asking.

But does mypy not have a more to automatically download the missing typings from typeshed?

It does download missing types, with --install-types, on the other hand it seemed better to list them explicitly and make sure they are present in mypy and pyright environments rather than leave it on linter guessing.

Good to keep this option in mind. If we end up decommissioning the pre-commit in favor of this workflow, that might be necessary to get the typeshed. At the very least it is worth making a temp commit running it with --install-types to see if it uses the restrictions as described in the previous comment

Edit: Did a bit of navigation around install_types and it seems it does not include the version constraint 1, and the typeshed packages are not version matched to the original package (the intent upstream there is that the typeshed have no executable code, including the original package). So some kind of logic to search for the missing stubs and manually create the requirements.txt might be needed.

Footnotes

  1. https://github.com/python/mypy/blob/43a605f742bd554acbdff9bea74c764621e3aa44/mypy/build.py#L2811

@happz happz force-pushed the tests-type-annotations-in-ci branch from 11a2dcc to 54ad9f3 Compare July 20, 2024 13:56
@happz happz marked this pull request as ready for review July 20, 2024 14:07
@happz happz added the command | tests tmt tests command label Jul 20, 2024
@happz happz added this to the 1.35 milestone Jul 20, 2024
@happz
Copy link
Collaborator Author

happz commented Jul 20, 2024

/packit test

@happz happz marked this pull request as draft July 21, 2024 12:21
@martinhoyer martinhoyer modified the milestones: 1.35, 1.36 Jul 30, 2024
tier: 0

adjust+:
- when: distro == centos-stream-9
Copy link
Collaborator

@martinhoyer martinhoyer Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed on rhel-9 btw. Do we need to keep compatibility with older releases, or can we get rid of the workaround completely?

EDIT: actually, not fixed yet. "Release pending" :)

environment+:
HATCH_ENVIRONMENT: dev-not-editable

require+:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can use a single source of truth for the list of the packages. Maybe something similar to the existing make develop perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could use make develop directly. However, it's a bit unsatisfactory solution, instead of a declaration of packages required by a test, we'd instead take action and run make. I'm going to switch to this make develop command for now, but maybe we need to save these requirements somewhere from which both test and make could read it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in dcd0402

@happz happz modified the milestones: 1.36, 1.37 Sep 2, 2024
The goal si to verify the code and its annotations are compatible with
Python packages shipped as system packages.
@happz happz force-pushed the tests-type-annotations-in-ci branch from 41a3b58 to dcd0402 Compare September 27, 2024 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution command | tests tmt tests command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants