Add optional requires_python to third party stubs metadata by hamdanal · Pull Request #10724 · python/typeshed

hamdanal · 2023-09-17T13:23:38Z

The typeshed part of #10722

This PR implements the following:

parse an optional requires_python field in the third party stubs
the field must be a valid Python version specifier that specifies the minimum Python version supported by the stub
the version must be at least the minimum version supported by typeshed (currently 3.7)
if the field exists, mypy, regression, and stubtest tests are skipped on Python versions that do not satisfy the requirement

stub-uploader still needs to learn about this field and include it in the setup.py file of the corresponding package with a fallback to the minimal version supported by typeshed.

AlexWaygood · 2023-09-17T13:34:57Z

tests/regr_test.py should also probably be updated so that it doesn't run the test cases for a package that has requires_python >= 3.9 and --python-version 3.8 is passed to the script.

It wouldn't be as easy to adjust pyright/pytype, but I also don't think it'll be as important to "fix" those tests in a "principled" way; we can probably leave them as they are for now

AlexWaygood · 2023-09-17T14:56:15Z

The changes to regr_test.py can probably be deferred to a followup PR, though; we don't need to do it all together

hamdanal · 2023-09-17T16:56:46Z

It wouldn't be as easy to adjust pyright/pytype, but I also don't think it'll be as important to "fix" those tests in a "principled" way; we can probably leave them as they are for now

Okay. I'll leave them for later

The changes to regr_test.py can probably be deferred to a followup PR, though; we don't need to do it all together

Let me see if it is simple enough to do here, if not I'll leave it as well

tests/parse_metadata.py

AlexWaygood

Lots of good stuff here, but needs a few changes -- see my comments below:

tests/parse_metadata.py

tests/regr_test.py

tests/parse_metadata.py

tests/mypy_test.py

tests/regr_test.py

tests/stubtest_third_party.py

- move to pyproject.toml - add to __all__ - parens in message - mininum supported version is also not allowed - delete print - test against python runtime version and target versions (mypy and regr tests) - different messages for different reasons - default the specifier to >=oldest_supported

tests/parse_metadata.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood · 2023-09-21T12:18:26Z

With this PR checked out, I tried applying this diff:

diff --git a/stubs/requests/METADATA.toml b/stubs/requests/METADATA.toml
index 39371b90e..677799c7d 100644
--- a/stubs/requests/METADATA.toml
+++ b/stubs/requests/METADATA.toml
@@ -1,6 +1,7 @@
 version = "2.31.*"
 upstream_repository = "https://github.com/psf/requests"
 requires = ["types-urllib3"]
+requires_python = ">=3.9"

 [tool.stubtest]
 extras = ["socks"]

I then ran this command (using Python 3.11):

python tests/mypy_test.py stubs/requests

Output:

*** Testing Python 3.12 on win32
Testing third-party packages...
testing requests (17 files)... success

*** Testing Python 3.11 on win32
Testing third-party packages...
testing requests (17 files)... success

*** Testing Python 3.10 on win32
Testing third-party packages...
testing requests (17 files)... success

*** Testing Python 3.9 on win32
Testing third-party packages...
testing requests (17 files)... success

*** Testing Python 3.8 on win32
Testing third-party packages...
skipping 'requests' for target Python 3.8 (requires Python >=3.9)
Traceback (most recent call last):
  File "C:\Users\alexw\coding\typeshed\tests\mypy_test.py", line 573, in <module>
    main()
  File "C:\Users\alexw\coding\typeshed\tests\mypy_test.py", line 556, in main
    code, files_checked_this_version, packages_skipped_this_version = test_typeshed(code, args=config, tempdir=td_path)
                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\typeshed\tests\mypy_test.py", line 535, in test_typeshed
    code, third_party_files_checked, third_party_packages_skipped = test_third_party_stubs(code, args, tempdir)
                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\typeshed\tests\mypy_test.py", line 511, in test_third_party_stubs
    assert _DISTRIBUTION_TO_VENV_MAPPING.keys() == distributions_to_check.keys()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

tests/mypy_test.py

tests/regr_test.py

AlexWaygood · 2023-09-21T12:36:25Z

Running this:

python tests/regr_test.py requests -p3.8

Results in this output being printed to the terminal:

skipping 'requests' for target Python 3.8 (requires Python >=3.9)

Test completed successfully!

Could we have it print something like "All tests were skipped!" to the terminal, if all tests were skipped? If all tests were skipped, that doesn't feel like a "successful" test run to me.

…sages

hamdanal · 2023-09-23T08:26:15Z

tests/mypy_test.py

+    # Some distributions may have been skipped earlier and therefore don't have a venv.
+    distributions_without_venv = {
+        distribution: requirements
+        for distribution, requirements in distributions_to_check.items()
+        if distribution not in _DISTRIBUTION_TO_VENV_MAPPING
+    }
+    if distributions_without_venv:
+        setup_virtual_environments(distributions_without_venv, args, tempdir)
+
+    # Check that there is a venv for every distribution we're testing.
+    # Some venvs may exist from previous runs but are skipped in this run.
+    assert _DISTRIBUTION_TO_VENV_MAPPING.keys() >= distributions_to_check.keys()
+
+    for distribution in distributions_to_check:
+        venv_info = _DISTRIBUTION_TO_VENV_MAPPING[distribution]


OK so this should work now in all cases (hopefully). I tested with your requests example and all these invocations work

python tests/mypy_test.py stubs/requests

python tests/mypy_test.py stubs/requests* # multiple packages together

python tests/mypy_test.py stubs/requests -p 3.8 3.9 3.10 3.11

python tests/mypy_test.py stubs/requests -p 3.11 3.8 3.9 3.10 # this also crashed before, order is important

python tests/mypy_test.py stubs/requests* -p 3.8 3.9 3.10 3.11

python tests/mypy_test.py stubs/requests* -p 3.11 3.8 3.9 3.10

Also the for loop now correctly skips distributions that should not run. It must loop over distributions_to_check, not venvs

This is part of the work towards python/typeshed#10722 Note that the field is called `python_requires` in `setup.py`. References: - typeshed issue: python/typeshed#10722 - typeshed PR: python/typeshed#10724 - setuptools docs: https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#python-requirement

AlexWaygood · 2023-09-23T15:37:04Z

Oh, shoot, I just caused you merge conflicts with #10714 -- sorry :(

I was planning on merging this first and then fixing the conflicts in my PR, but I forgot :(

hamdanal · 2023-09-23T15:43:30Z

Oh, shoot, I just caused you merge conflicts with #10714 -- sorry :(

I was planning on merging this first and then fixing the conflicts in my PR, but I forgot :(

No problem, I fixed them.

I removed the product loop and used separate for loops. This allows us to minimize the number of version checks we do and the number of messages we print for skipped tests. Now we print one "skipped" message per version only.

hamdanal · 2023-09-23T16:23:44Z

@AlexWaygood WDYT about this patch?

diff --git a/tests/parse_metadata.py b/tests/parse_metadata.py
index dfb266ad0..9977db339 100644
--- a/tests/parse_metadata.py
+++ b/tests/parse_metadata.py
@@ -7,7 +7,7 @@ from __future__ import annotations
 import os
 import re
 import urllib.parse
-from collections.abc import Mapping
+from collections.abc import Callable, Mapping
 from dataclasses import dataclass
 from pathlib import Path
 from typing import NamedTuple
@@ -18,7 +18,7 @@ from packaging.requirements import Requirement
 from packaging.specifiers import Specifier
 from packaging.version import Version
 
-from utils import cache
+from utils import PYTHON_VERSION, cache, colored
 
 __all__ = [
     "NoSuchStubError",
@@ -129,6 +129,7 @@ class StubMetadata:
     Don't construct instances directly; use the `read_metadata` function.
     """
 
+    name: str
     version: str
     requires: Annotated[list[str], "The raw requirements as listed in METADATA.toml"]
     extra_description: str | None
@@ -141,6 +142,21 @@ class StubMetadata:
     stubtest_settings: StubtestSettings
     requires_python: Annotated[Specifier, "Versions of Python supported by the stub package"]
 
+    def skip_test(self, version: str | None = None, printer: Callable[[str], object] = print) -> bool:
+        """Return True if the stub should be skipped.
+
+        If version is None, the runtime Python version is used.
+        """
+        if self.requires_python.contains(version if version is not None else PYTHON_VERSION):
+            return False
+        msg = f"skipping {self.name!r} "
+        if version is not None:
+            msg += f"for target Python {version} (requires Python {self.requires_python})"
+        else:
+            msg += f"(requires Python {self.requires_python}; test is being run using Python {PYTHON_VERSION})"
+        printer(colored(msg, "yellow"))
+        return True
+
 
 _KNOWN_METADATA_FIELDS: Final = frozenset(
     {
@@ -277,6 +293,7 @@ def read_metadata(distribution: str) -> StubMetadata:
             assert key in tk, f"Unrecognised {tool} key {key!r} for {distribution!r}"
 
     return StubMetadata(
+        name=distribution,
         version=version,
         requires=requires,
         extra_description=extra_description,

since we are doing this check in a lot of places. Then we can simply do:

if metadata.skip_test():
    continue

to check against runtime Python version or

if metadata.skip_test(version):
    continue

to check against target version from the command line

AlexWaygood · 2023-09-23T17:07:59Z

@AlexWaygood WDYT about this patch?

Hmm, not sure -- I think I prefer it as you currently have it. That patch leads to more shared code, it's true, but it feels strange to me to be printing stuff to the terminal from parse_metadata.py. The idea of that module is that it contains common logic to help abstract parsing the METADATA.toml files and provide validation for security purposes. I feel like those functions should be ignorant of how they're being used and of things like the specific Python version they're being run on.

I wouldn't be opposed to adding a little helper method, though, e.g.

class StubMetadata:
    def should_skip_tests(py_version: str) -> bool:
        return self.requires_python.contains(version)

tests/mypy_test.py

tests/regr_test.py

hamdanal · 2023-09-23T17:14:11Z

It is strange hence my question :D
In this case let's keep things as they are, self.requires_python.contains(version) is expressive enough IMO.

tests/regr_test.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

tests/mypy_test.py

tests/regr_test.py

AlexWaygood

Other than #10724 (comment), this now looks great. Thanks for persevering with this, there was a lot to think through here!

AlexWaygood · 2023-09-24T17:21:06Z

@srittau, did you want to take a quick look, or are you okay with me merging this?

srittau · 2023-09-24T17:35:03Z

I haven't had time to look at it, but I don't need to. Please merge if you are confident.

AlexWaygood · 2023-09-24T17:39:41Z

Thanks again @hamdanal! :D

hamdanal · 2023-09-24T18:50:33Z

Thank you Alex, as always, you've been very helpful :)

Avasam · 2023-09-24T19:48:22Z

Thanks! My only issue, is that it's not documented in https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#the-metadatatoml-file !

AlexWaygood · 2023-09-24T19:51:12Z

Thanks! My only issue, is that it's not documented in https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#the-metadatatoml-file !

A very good point, that I completely missed!

AlexWaygood · 2023-09-24T19:57:00Z

Thanks! My only issue, is that it's not documented in https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#the-metadatatoml-file !

Added it to the to-do list in #10722 (comment)

WIP

eee2770

hamdanal added 2 commits September 17, 2023 19:20

Also check in regr_test

db9b3ca

mypy

1e1c1dc

hamdanal commented Sep 17, 2023

View reviewed changes

tests/parse_metadata.py Show resolved Hide resolved

check specifier operator

32625b3

hamdanal marked this pull request as ready for review September 17, 2023 17:40

AlexWaygood reviewed Sep 19, 2023

View reviewed changes

hamdanal commented Sep 20, 2023

View reviewed changes

tests/parse_metadata.py Outdated Show resolved Hide resolved

hamdanal and others added 4 commits September 20, 2023 22:45

Merge remote-tracking branch 'upstream/main' into requires-python

0b34da8

Merge branch 'main' into requires-python

6fd2dfc

Alex's suggestion

549ff52

Move to utils

ac949cd

AlexWaygood reviewed Sep 20, 2023

View reviewed changes

tests/parse_metadata.py Outdated Show resolved Hide resolved

tests/parse_metadata.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ef749c4

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood reviewed Sep 21, 2023

View reviewed changes

tests/mypy_test.py Outdated Show resolved Hide resolved

AlexWaygood reviewed Sep 21, 2023

View reviewed changes

tests/mypy_test.py Outdated Show resolved Hide resolved

tests/regr_test.py Outdated Show resolved Hide resolved

Avasam mentioned this pull request Sep 23, 2023

[stubsabot] Bump pyinstaller to 6.0.* #10752

Closed

hamdanal added 2 commits September 23, 2023 10:17

Fix assertion error, actually skip mypy test when needed, improve mes…

7b1043e

…sages

Merge remote-tracking branch 'upstream/main' into requires-python

1499be0

hamdanal commented Sep 23, 2023

View reviewed changes

This was referenced Sep 23, 2023

Add a "requires-python" metadata field in stub distributions typeshed-internal/stub_uploader#103

Merged

Stubs that require a version of python higher than the minimum supported by typeshed #10722

Open

type ignore mypy happiness

585fc7c

Merge remote-tracking branch 'upstream/main' into requires-python

e070dc9

AlexWaygood reviewed Sep 23, 2023

View reviewed changes

tests/mypy_test.py Outdated Show resolved Hide resolved

tests/regr_test.py Show resolved Hide resolved

tests/regr_test.py Outdated Show resolved Hide resolved

CR

515fe80

AlexWaygood reviewed Sep 23, 2023

View reviewed changes

tests/regr_test.py Outdated Show resolved Hide resolved

AlexWaygood reviewed Sep 23, 2023

View reviewed changes

tests/regr_test.py Outdated Show resolved Hide resolved

hamdanal commented Sep 23, 2023

View reviewed changes

tests/regr_test.py Outdated Show resolved Hide resolved

Apply suggestions from code review

9ae5239

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood reviewed Sep 23, 2023

View reviewed changes

tests/mypy_test.py Outdated Show resolved Hide resolved

Simplify

4a14f42

AlexWaygood mentioned this pull request Sep 24, 2023

Revert "Do a dry run of stubsabot on PRs touching scripts/ or tests/" #10754

Closed

AlexWaygood reviewed Sep 24, 2023

View reviewed changes

tests/regr_test.py Show resolved Hide resolved

AlexWaygood approved these changes Sep 24, 2023

View reviewed changes

Update tests/regr_test.py

b4b6e5c

AlexWaygood merged commit c5dde1e into python:main Sep 24, 2023

hamdanal deleted the requires-python branch September 24, 2023 18:49

hamdanal mentioned this pull request Sep 24, 2023

Add seaborn stubs #10721

Merged

3 tasks

AlexWaygood mentioned this pull request Sep 26, 2023

Declare that types-jsonschema requires Python 3.8+ #10775

Merged

AlexWaygood mentioned this pull request Oct 11, 2023

Fix mypy_test if mypy returns negative exit code #10866

Merged

Uh oh!

Conversation

hamdanal commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented Sep 17, 2023

Uh oh!

hamdanal commented Sep 17, 2023

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamdanal Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamdanal commented Sep 23, 2023

Uh oh!

hamdanal commented Sep 23, 2023

Uh oh!

AlexWaygood commented Sep 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hamdanal commented Sep 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Sep 24, 2023

Uh oh!

srittau commented Sep 24, 2023

Uh oh!

AlexWaygood commented Sep 24, 2023

Uh oh!

hamdanal commented Sep 24, 2023

Uh oh!

Avasam commented Sep 24, 2023

Uh oh!

AlexWaygood commented Sep 24, 2023

Uh oh!

AlexWaygood commented Sep 24, 2023

Uh oh!

Reviewers

Assignees

Labels

hamdanal commented Sep 17, 2023 •

edited

Loading

AlexWaygood commented Sep 17, 2023 •

edited

Loading

AlexWaygood commented Sep 21, 2023 •

edited

Loading

AlexWaygood commented Sep 21, 2023 •

edited

Loading

hamdanal Sep 23, 2023 •

edited

Loading

AlexWaygood commented Sep 23, 2023 •

edited

Loading

AlexWaygood left a comment •

edited

Loading