Skip to content

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

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
hamdanal opened this issue Sep 17, 2023 · 14 comments
Labels
project: infrastructure typeshed build, test, documentation, or distribution related

Comments

@hamdanal
Copy link
Contributor

In #10721, the seaborn stubs depend on matplotlib that only included a py.typed marker file in version 3.8.
matplotlib version 3.8 requires python >=3.9 which leads to the mypy tests failing on python 3.8.

Alex Waygood suggests adding a requires-python field to METADATA.toml and teaching stub-uploader to include it in the generated setup.py #10721 (comment).

Please let us know what you think about this idea or if you have another solution.

@AlexWaygood AlexWaygood added the project: infrastructure typeshed build, test, documentation, or distribution related label Sep 17, 2023
@AlexWaygood
Copy link
Member

I think this would be a good idea.

We actually already have a need for this: typeshed claims to support Python 3.7+, but the latest version of types-jsonschema will only work with mypy on Python 3.8+. The only reason why this isn't causing our CI to fail is that we no longer run mypy in CI on Python 3.7, because the latest version of mypy only supports Python 3.8+. But that isn't particularly principled; we should signal to our users that the stubs package we're shipping only works with Python 3.8+, by including that information in the generated setup.py file.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 17, 2023

Oh, actually, the types-jsonschema PR I was thinking of hasn't been merged yet... the package will require Python 3.8+ if #10583 is merged :-)

@hamdanal
Copy link
Contributor Author

Should the field be empty by default or should it be set to typeshed's minimum supported python?

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 17, 2023

The METADATA.toml field should be optional — the default assumption should be that most stubs packages can be used with all versions of Python that typeshed supports.

We should add some validation to tests/parse_metadata.py to ensure that we don't have any requires-python fields that specify a minimum Python version less than the oldest version of Python supported by typeshed.

I'm not sure what the stub uploader should do if it encounters a stubs package that doesn't have a requires-python field in METADATA.toml. Should it add requires-python to the generated setup.py file anyway, and set it to the lowest version of Python supported by typeshed? Or should it just not add anything to the setup.py file?

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 17, 2023

I'm not sure what the stub uploader should do if it encounters a stubs package that doesn't have a requires-python field in METADATA.toml. Should it add requires-python to the generated setup.py file anyway, and set it to the lowest version of Python supported by typeshed? Or should it just not add anything to the setup.py file?

Answering my own question after thinking about this a little more: I think it should add requires-python to the generated setup.py file anyway, and set it to the lowest version of Python supported by typeshed. Here's why: in January 2024, we'll be dropping support for Python 3.7 (#10113), and when we do so, we'll likely start using PEP-570 syntax across most of typeshed. That will make most of our stubs packages incompatible with Python 3.7, even the ones that don't have any external dependencies. It will be important to signal this to our users.

@hamdanal
Copy link
Contributor Author

That will make most of our stubs packages incompatible with Python 3.7, even the ones that don't have any external dependencies. It will be important to signal this to our users.

Makes sense.

hamdanal added a commit to hamdanal/stub_uploader that referenced this issue Sep 23, 2023
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
@hamdanal
Copy link
Contributor Author

hamdanal commented Sep 23, 2023

Required tasks:

@Avasam
Copy link
Collaborator

Avasam commented Sep 29, 2023

Since this issue touches that area.
Example scenario that we'd like to avoid in runtests and that using per-stubs python version would help with instead of always running on 3.8:
https://github.com/python/typeshed/actions/runs/6347797998/job/17243343457?pr=10801
I imported Final from the wrong location, ran the scripts locally which told me it was all good on Python 3.8+. But it failed on the CI on Python 3.7.

@hamdanal
Copy link
Contributor Author

I just took a quick look on the pyright and pytype scripts. The problem here is that these tests run on the whole project. They have to become modular somehow before applying the same logic of skipping tests in mypy and stubtest. This is a bigger change than what we did with mypy.

As for the specific issue you got, maybe there is a ruff rule that replaces or bans certain imports?

hamdanal added a commit to hamdanal/typeshed that referenced this issue Sep 30, 2023
@AlexWaygood
Copy link
Member

I just took a quick look on the pyright and pytype scripts. The problem here is that these tests run on the whole project. They have to become modular somehow before applying the same logic of skipping tests in mypy and stubtest. This is a bigger change than what we did with mypy.

Yes, to explain the context here: as you noted, we run pytype and pyright pretty differently in CI to how to we run mypy (whether that's mypy_test.py, stubtest, or regr_test.py).

With our mypy tests, we do very elaborate things to try to ensure that each stubs package is only tested in isolation. That means that if stubs package A states that it requires numpy but stubs package B doesn't, they'll be tested with mypy using different virtual environments so that it's detected in CI if (for example) we forget to declare that stubs package A depends on numpy. This is the "principled" way of doing things, but leads to some fairly complex logic in the test scripts, and also makes the scripts somewhat slow. (That can be mitigated using threading/multiprocessing, but that only makes the scripts more complicated 😆.)

When we first added the infrastructure to typeshed allowing non-types dependencies, it was decided that we'd only do this kind of thing for mypy's test scripts. For the other type checkers we run in CI, we'd take a less principled, but simpler, approach: we'd just install all our non-types dependencies together into a big soup, and then test all our stubs packages together. Since we already do the principled thing with mypy, it was thought that it probably wouldn't catch any more bugs if we also did the principled/complex thing with the other two type checkers we run in CI, so it probably wasn't worth the maintenance cost to attempt to do so.

@AlexWaygood
Copy link
Member

I imported Final from the wrong location, ran the scripts locally which told me it was all good on Python 3.8+. But it failed on the CI on Python 3.7.

Yeah, that's basically just because we've got a slightly unusual situation going on right now where we've partially-but-not-completely dropped support for Python 3.7. Some, but not all of our tests, run with --python-version 3.7 in CI now, and none of them will come January.

If you want a fix @Avasam, I'd merge a PR that did this, since we can still run our pyright tests with --pythonversion 3.7 in CI -- it's just mypy that we can no longer run with Python 3.7:

diff --git a/scripts/runtests.py b/scripts/runtests.py
index d7aebb9e5..b8ff184b0 100644
--- a/scripts/runtests.py
+++ b/scripts/runtests.py
@@ -96,7 +96,7 @@ def main() -> None:
     strict_params = _get_strict_params(path)
     print(f"\nRunning Pyright ({'stricter' if strict_params else 'base' } configs) for Python {_PYTHON_VERSION}...")
     pyright_result = subprocess.run(
-        [sys.executable, "tests/pyright_test.py", path, "--pythonversion", _PYTHON_VERSION] + strict_params,
+        [sys.executable, "tests/pyright_test.py", path, "--pythonversion", "3.7"] + strict_params,
         stderr=subprocess.PIPE,
         text=True,
     )

But also, January isn't too far away, so we could probably just wait until then, when the problem will fix itself as a result of us dropping support for Python 3.7 :)

@hamdanal
Copy link
Contributor Author

hamdanal commented Oct 1, 2023

With our mypy tests, we do very elaborate things to try to ensure that each stubs package is only tested in isolation. That means that if stubs package A states that it requires numpy but stubs package B doesn't, they'll be tested with mypy using different virtual environments so that it's detected in CI if (for example) we forget to declare that stubs package A depends on numpy. This is the "principled" way of doing things, but leads to some fairly complex logic in the test scripts, and also makes the scripts somewhat slow. (That can be mitigated using threading/multiprocessing, but that only makes the scripts more complicated 😆.)

Thanks for the context.

I think moving forward the whole testing process needs to be simplified, otherwise every addition will require a lot of tricky stuff and duplication of code. One way of dealing with this could be to factor out the logic that parses command line arguments to get which files need to be checked, the version checks + skipping, virtual environment creation and caching, and some other common tasks like results reporting, so that they can be used by all checks. We could maybe even go with factoring out the common bits to a state where adding a new check (new type checker!) could be made with a simple change to a subprocess call.

The virtual environment creation specifically could be made its own job in CI (with proper caching even between runs) that other jobs depend on. For local tests, this can also be done perhaps with an in-project build of virtual envs that are persistent on disk until a dependency changes or some time passes since creation. This means any execution of mypy et al, pyright, pytype would create the venv once and reuse it.

I don’t think I’ll have time to go through all of this in the near future but hopefully someone else does. I’ll help with this whenever I get a chance.

@AlexWaygood
Copy link
Member

I think moving forward the whole testing process needs to be simplified, otherwise every addition will require a lot of tricky stuff and duplication of code. One way of dealing with this could be to factor out the logic that parses command line arguments to get which files need to be checked, the version checks + skipping, virtual environment creation and caching, and some other common tasks like results reporting, so that they can be used by all checks. We could maybe even go with factoring out the common bits to a state where adding a new check (new type checker!) could be made with a simple change to a subprocess call.

Yeah. I've previously tried to somewhat consolidate our logic between our test scripts in PRs such as #8700, #9382 and #9534. It's possible there's more still that we could do. However, it's tricky to centralise logic much more IMO, because the scripts all work pretty differently (for a number of reasons, among them: the different type checkers work differently; the different scripts have different aims; and the different scripts were created by different people at different points in time rather than all together at the same time). There's also the risk that we try to abstract away the differences between the scripts too much, which can make it harder to read the code and harder to refactor the scripts. Some of the functions that I've previously added to tests/utils.py already fall foul of this a little bit, I think: in hindsight, I'm not sure the print_success_msg() function actually improves anything. We could probably just inline that logic in the two places in our tests where the function is called, and the code would probably be more readable tbh.

Having said all that, though, you're very welcome to try to improve things further! :D

The virtual environment creation specifically could be made its own job in CI (with proper caching even between runs) that other jobs depend on. For local tests, this can also be done perhaps with an in-project build of virtual envs that are persistent on disk until a dependency changes or some time passes since creation. This means any execution of mypy et al, pyright, pytype would create the venv once and reuse it.

This is something I've been dreaming of for a while. You could maybe have a special directory for storing virtual envs for each stubs project (.mypy-test-venvs for mypy_test.py, .stubtest-venvs for stubtest_third_party.py, etc.). Every time mypy_test.py is invoked (for example), it could check whether each venv in .mypy-test-venvs is up to date, check whether any need to be added or deleted, and update the existing ones if necessary (maybe using pip-tools?). If they're all up to date, it won't need to do anything, it can just reuse them from the last invocation of the script. It could potentially be much faster than the current approach.

But, I haven't got around to actually doing anything like that yet :) And, of course... it will only make the scripts even more complicated. Sigh.

@AlexWaygood
Copy link
Member

Note that when it comes to pyright, we don't even use a script in CI. We have tests/pyright_test.py, and we used to use that in CI, but nowadays it's purely for local testing. We currently use the pyright-action GitHub Action to invoke pyright in CI:

pyright:
name: Test typeshed with pyright
runs-on: ubuntu-latest
strategy:
matrix:
python-platform: ["Linux", "Windows", "Darwin"]
python-version: ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"]
fail-fast: false
steps:
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: "3.11"
cache: pip
cache-dependency-path: requirements-tests.txt
- name: Install typeshed test-suite requirements
# Install these so we can run `get_external_stub_requirements.py`
run: pip install -r requirements-tests.txt
- name: Create an isolated venv for testing
run: python -m venv .venv
- name: Install 3rd-party stub dependencies
run: |
DEPENDENCIES=$(python tests/get_external_stub_requirements.py)
if [ -n "$DEPENDENCIES" ]; then
source .venv/bin/activate
echo "Installing packages: $DEPENDENCIES"
pip install $DEPENDENCIES
fi
- name: Activate the isolated venv for the rest of the job
run: echo "$PWD/.venv/bin" >> $GITHUB_PATH
- name: List 3rd-party stub dependencies installed
run: pip freeze --all
- name: Get pyright version
uses: SebRollen/toml-action@v1.0.2
id: pyright_version
with:
file: "pyproject.toml"
field: "tool.typeshed.pyright_version"
- name: Run pyright with basic settings on all the stubs
uses: jakebailey/pyright-action@v1
with:
version: ${{ steps.pyright_version.outputs.value }}
python-platform: ${{ matrix.python-platform }}
python-version: ${{ matrix.python-version }}
no-comments: ${{ matrix.python-version != '3.11' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
- name: Run pyright with stricter settings on some of the stubs
uses: jakebailey/pyright-action@v1
with:
version: ${{ steps.pyright_version.outputs.value }}
python-platform: ${{ matrix.python-platform }}
python-version: ${{ matrix.python-version }}
no-comments: ${{ matrix.python-version != '3.11' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
project: ./pyrightconfig.stricter.json
- name: Run pyright on the test cases
uses: jakebailey/pyright-action@v1
with:
version: ${{ steps.pyright_version.outputs.value }}
python-platform: ${{ matrix.python-platform }}
python-version: ${{ matrix.python-version }}
no-comments: ${{ matrix.python-version != '3.11' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
project: ./pyrightconfig.testcases.json

hamdanal added a commit to hamdanal/typeshed that referenced this issue Oct 13, 2023
This makes the script usable for packages that do not support the minimum
python version hardcoded in the script.

Another part of python#10722
srittau pushed a commit that referenced this issue Oct 13, 2023
This makes the script usable for packages that do not support the minimum
python version hardcoded in the script.

Another part of #10722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: infrastructure typeshed build, test, documentation, or distribution related
Projects
None yet
Development

No branches or pull requests

3 participants