mypy_test.py: Allow non-types dependencies
#9408
Conversation
AlexWaygood
force-pushed
the
mypy-test-non-types
branch
from
2037d09
to
ca484c4
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This reverts commit db260f2.
|
Okay, I think this is ready for review. The approach is pretty similar to the approach I took in #9382 for I mitigate the performance issues associated with dynamically creating virtual environments in two ways:
The CI run on db260f2 shows what happens if we add non-types requirements to several packages in typeshed. I added I've also confirmed that the mypy tests all pass on #9459 locally if this PR is applied. |
| def get_mypy_flags(args: TestConfig, temp_name: str, *, testing_stdlib: bool) -> list[str]: | ||
| flags = [ | ||
| "--python-version", | ||
| args.version, | ||
| "--show-traceback", | ||
| "--warn-incomplete-stub", | ||
| "--show-error-codes", | ||
| "--no-error-summary", | ||
| "--platform", | ||
| args.platform, | ||
| "--no-site-packages", | ||
| "--custom-typeshed-dir", | ||
| str(Path(__file__).parent.parent), | ||
| "--strict", | ||
| # Stub completion is checked by pyright (--allow-*-defs) | ||
| "--allow-untyped-defs", | ||
| "--allow-incomplete-defs", | ||
| "--allow-subclassing-any", # Needed until we can use non-types dependencies #5768 | ||
| "--enable-error-code", | ||
| "ignore-without-code", | ||
| "--config-file", | ||
| temp_name, | ||
| ] | ||
| if not testing_stdlib: | ||
| flags.append("--explicit-package-bases") | ||
| return flags |
There was a problem hiding this comment.
I moved these inline as it honestly didn't make much sense for this to be a separate function any more. The precise set of flags that's being passed now depends on whether it's the stdlib being tested, and (if it's a third-party package) whether the stubs package depends (either directly or indirectly) on a non-types package.
| @@ -142,7 +145,7 @@ def make_venv(venv_dir: Path) -> VenvInfo: | |||
| try: | |||
| venv.create(venv_dir, with_pip=True, clear=True) | |||
| except subprocess.CalledProcessError as e: | |||
| if "ensurepip" in e.cmd: | |||
| if "ensurepip" in e.cmd and b"KeyboardInterrupt" not in e.stdout.splitlines(): | |||
There was a problem hiding this comment.
The "if you're on Linux, you might need to install the python3-venv package" message was showing up every time I interrupted the script with a KeyboardInterrupt during the "concurrently create many venvs" step, and it was kinda annoying.
…package and the dependency has a lower or upper bound
|
|
||
|
|
||
| def install_requirements_for_venv(venv_info: VenvInfo, args: TestConfig, external_requirements: frozenset[str]) -> None: | ||
| # Use --no-cache-dir to avoid issues with concurrent read/writes to the cache |
There was a problem hiding this comment.
Doesn't GitHub Actions provide a way to cache pip installs across runs? If that's available, it would be a pretty promising way to optimize this check.
There was a problem hiding this comment.
It does, yeah. But it's nice to be able to run this script locally as well, and for that we need a solution that's performant even if it doesn't have access to the GitHub Actions cache. I'm also not entirely sure if the GitHub Actions cache is accessible from within Python scripts or not. (I have no idea how it works tbh.)
Anyway, I'll merge this for now, and if anybody can think of further optimisations, they can be applied in future PRs :)
Thanks for the review!
There was a problem hiding this comment.
@JelleZijlstra If you mean the setup-python action. It only caches the download, not the install. See actions/setup-python#330
It's still possible to do it manually using actions/cache .
Some fairly subtle logic was added in #9408, and I'm immediately regretting not having documented it slightly better. This PR adds some more comments to make maintaining this code easier in the future.
Some fairly subtle logic was added in #9408, and I'm immediately regretting not having documented it slightly better. This PR adds some more comments to make maintaining this code easier in the future.
Work towards #5768 (cc. @Avasam)