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

Support third-party stub external dependencies in pytype #9449

Merged
merged 7 commits into from
Jan 4, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Jan 3, 2023

Work towards #5768
Based on, and mostly a copy of what's done in, #9434 (hence I'm keeping this as draft until the pyright PR is merged)

I'll revert the demo changes in METADATA.toml after the above is merged.

The pytype failure seen in #9374 seems to be caused by non-typed external dependencies, which we decided we won't support at the moment anyway.

@@ -17,7 +17,18 @@ objects at runtime.
in the `tests` and `scripts` directories.

To run the tests, follow the [setup instructions](../CONTRIBUTING.md#preparing-the-environment)
in the `CONTRIBUTING.md` document. In particular, we recommend running with Python 3.9+.
in the `CONTRIBUTING.md` document. In particular, you have to run with Python 3.9+.
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 changed the wording here because since #9382, test/utils.py (which is used by all tests) uses functools.cache, which is unavailable in Python 3.8.

Copy link
Member

Choose a reason for hiding this comment

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

In practice we've required 3.9+ for a lot longer, due to the use of ast.unparse in tests/check_new_syntax.py :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good to know. I believe you could still run other tests, so "recommend" wasn't too inaccurate :P

I only noticed because I had 3.8 installed in WSL.

in the `CONTRIBUTING.md` document. In particular, we recommend running with Python 3.9+.
in the `CONTRIBUTING.md` document. In particular, you have to run with Python 3.9+.

In order for `pytype_test` and `pyright_test` to work correctly, some third-party stubs
Copy link
Collaborator Author

@Avasam Avasam Jan 4, 2023

Choose a reason for hiding this comment

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

Same wording as before, just added "pytype_test and" moved it up to avoid duplicating this entire paragraph.

@Avasam Avasam marked this pull request as ready for review January 4, 2023 00:12
@Avasam
Copy link
Collaborator Author

Avasam commented Jan 4, 2023

Successful pytype run: https://github.com/python/typeshed/actions/runs/3833991938/jobs/6525955844

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Some very minor points below

.github/workflows/tests.yml Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 4, 2023

@rchen152, I'd love your opinion on this as a pytype maintainer, if you've the time.

We're trying to rework our test suite to allow our third-party stubs packages to have dependencies on non-stubs packages external to typeshed which ship with py.typed files. For mypy_test.py, my proposed approach in #9408 is to dynamically create virtual environments in order to test each stubs package in isolation. For pyright, however, we went with #9434, which simply installs all dependencies for all third-party stubs into one environment before testing them all together.

The pyright approach has the advantage that the test code is much simpler and faster, but could lead to false negatives. If you add import numpy in a stubs package, but forget to declare a dependency on numpy for that package, pyright might not emit an error due to numpy already being installed for some other stubs package that we're testing. The mypy approach doesn't suffer from this flaw.

We could probably go either way when it comes to running pytype in CI — the "simple" approach we're using for pyright, or the "somewhat more principled" approach I'm proposing for mypy. The proposal in this PR is to use the pyright approach. Do you have any preference at all?

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll wait a little bit to see if the pytype maintainers have any thoughts before merging :)

@rchen152
Copy link
Collaborator

rchen152 commented Jan 4, 2023

Thanks for looping me in! This change looks good to me =) If mypy is already testing each package in isolation, it seems reasonable to use the "simple" approach for pytype.

@AlexWaygood
Copy link
Member

Brilliant, thanks for taking a look!

@AlexWaygood AlexWaygood merged commit 70025c3 into python:main Jan 4, 2023
@Avasam Avasam deleted the pytype-external-dependencies branch January 4, 2023 19:35
juanamari94 pushed a commit to juanamari94/typeshed that referenced this pull request Jan 6, 2023
juanamari94 pushed a commit to juanamari94/typeshed that referenced this pull request Jan 6, 2023
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 this pull request may close these issues.

3 participants