Improve how we run pyright in CI

[AlexWaygood]

Use the new --pythonpath command-line argument, instead of the --venv-path command-line argument in combination with the venv configuration setting. This is simpler, and also means that we don't need to hardcode the name of our virtual environment in our pyrightconfig.json files (which annoyed @Akuli in #10121). The --venv-path argument is also deprecated.

[AlexWaygood]

@erictraut, are the CI failures expected here? Maybe I misunderstood the solution you were recommending for typeshed in microsoft/pyright#5109. It looks as though pyright isn't picking up the PEP-561 packages installed into a virtual environment when the virtual environment is specified via --pythonpath on the command line.

[erictraut]

I don't see an obvious problem with this. The import failures are related to flask, numpy, and cryptography.hazmat. Where are these stubs being installed in the venv? I don't see them referenced in requirements-tests.txt. If you (temporarily) add the --verbose flag, pyright will emit a bunch of extra logging that could help diagnose the problem. For example, it will output all of the import resolution paths that it gets from the python environment (i.e. the value of sys.path).

[AlexWaygood]

I don't see an obvious problem with this. The import failures are related to flask, numpy, and cryptography.hazmat. Where are these stubs being installed in the venv? I don't see them referenced in requirements-tests.txt.

flask, numpy and cryptography are all installed in this step of the workflow here:

typeshed/.github/workflows/tests.yml

Lines 153 to 160 in 7df870f

	- 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

We know they are installed successfully because of the output of pip freeze in this step of the workflow here:

typeshed/.github/workflows/tests.yml

Lines 161 to 164 in 7df870f

	- name: List 3rd-party stub dependencies installed
	run: |
	source .venv/bin/activate
	pip freeze --all

https://github.com/python/typeshed/actions/runs/5169903229/jobs/9312454028?pr=10258

[AlexWaygood]

If you (temporarily) add the --verbose flag, pyright will emit a bunch of extra logging that could help diagnose the problem. For example, it will output all of the import resolution paths that it gets from the python environment (i.e. the value of sys.path).

It doesn't look like the --verbose option can be specified if you're using https://github.com/jakebailey/pyright-action. I tried adding it, but there's now an error message in CI saying:

'outputjson' option cannot be used with 'verbose' option
Error: Exit code 4

https://github.com/python/typeshed/actions/runs/5170067467/jobs/9312734659?pr=10258

From my reading of the docs for pyright-action, it doesn't look like there's any way of switching off the outputjson option while using that GitHub action.

[erictraut]

I forgot that the action is using --outputjson. Yeah, that won't work with --verbose.

Hmm, it looks correct to me.

I've never used the extra-args option in the pyright-action, so I wonder if that's the problem? I've reviewed the code, and I don't see any obvious issue. Maybe we could get @jakebailey to add a config option for the new pythonpath command-line option and publish a v2 of the action.

I've tried the steps locally (creating a local .venv, populating it with the dependencies, and then running pyright with the --pythonpath option), and it works fine.

[jakebailey]

I've barely rolled out of bed so haven't looked at this yet (besides typing on my phone), but I'm guessing the problem here are the quotes. The action executes pyright directly (via fork) and so the quotes showing up in the GHA log are on fact being passed literally in argv, whereas normally a shell would have stripped them and escaped their contents.

If you drop the quotes, I'm betting it will work.

That being said, I will go and add in the shorthand for the new flags, and also fix quotes in args; my intended behavior of extra-args was that quotes worked.

[AlexWaygood]

Thanks @jakebailey, looks like dropping the quotes fixed the issue with pyright not picking up the packages installed into the virtual environment!

However, looks like we still have a different error where pyright is now not recognising typeshed's VERSIONS file. @erictraut, could the --pythonpath option be interfering with the typeshedPath config option we set in our pyrightconfig.json files?

typeshed/pyrightconfig.json

Line 3 in 7df870f

"typeshedPath": ".",

[jakebailey]

From my reading of the docs for pyright-action, it doesn't look like there's any way of switching off the outputjson option while using that GitHub action.

Just to note it, this is also probably a bug in the action; the intent is that when a flag is set that prevents JSON from working (e.g. --verifytypes) that I disable JSON output and fall back to plain text without any cool features. But, I have to hardcode that list (along with the other flag shorthand). So, probably just my oversight.

[erictraut]

The VERSIONS file was recently updated to set the max version of some stubs to Python 3.11. This includes asynchat, asyncore and distutils.

It looks like the CI script is running pyright --pythonversion 3.12 on these files even though they are not intended for use with Python 3.12. These files attempt to import other files that are not available on Python 3.12, which explains the "Stub file not found" error messages. These errors look correct to me. Does that analysis sound correct to you, @AlexWaygood? If so, then I think the CI script will need to be updated to avoid running type checking on stub files that are invalid for a given Python version.

If I'm correct in my analysis and these pyright errors are legitimate, then why didn't we see these errors prior to this PR? I have a theory that I'm still investigating. The original import resolution logic in pyright was very precise. If the import couldn't be found using any of the configured paths, it results in an import resolution error. A couple of years ago, we (actually, Jake) added code to make this a bit more forgiving. Pyright now attempts to find a reasonable import by walking up the directory structure starting from the location of the file that's doing the import and stopping at the root of the project. It appears that this "fuzzy import" mechanism was masking these import resolution errors prior to this PR. I haven't yet determined why this behavior changed when using --pythonpath. Still investigating.

[AlexWaygood]

It looks like the CI script is running pyright --pythonversion 3.12 on these files even though they are not intended for use with Python 3.12. These files attempt to import other files that are not available on Python 3.12, which explains the "Stub file not found" error messages. These errors look correct to me. Does that analysis sound correct to you, @AlexWaygood?

This is nearly correct, but there's an important detail that's missing from this analysis. Take the first error in CI here, for example:

/home/runner/work/typeshed/typeshed/stdlib/asynchat.pyi:1:8 - error: Stub file not found for "asyncore" (reportMissingTypeStubs)

Pyright is correctly identifying here (from our VERSIONS file) that asyncore (which is being imported in asynchat.pyi) has been removed in Python 3.12. But what it's missing is that asynchat.pyi (the file it's checking) has also been removed in Python 3.12. So it shouldn't really matter that asynchat is importing asyncore, given that they're both gone in Python 3.12! It shouldn't actually be checking asynchat.pyi at all if --python-version 3.12 is specified, I think?

EDIT: we might be saying the same thing in different ways here, re-reading your message. The behaviour we get without using --pythonpath is definitely more convenient for us here, whatever the case :)

[erictraut]

The VERSIONS support in pyright applies only to import resolution. It doesn't magically exclude files from the project. If you want pyright not to analyze a given file under the root directory of your project, you'd need to exclude it from the project. You can do this by specifying an exclude section in the config file.

How does this work with other type checkers?

[AlexWaygood]

The VERSIONS support in pyright applies only to import resolution. It doesn't magically exclude files from the project. If you want pyright not to analyze a given file under the root directory of your project, you'd need to exclude it from the project. You can do this by specifying an exclude section in the config file.

Yeah, makes sense, and I realised shortly after posting my previous message that this was probably what you were saying. What's interesting, as you say, is that without the --pythonpath flag pyright gives the appearance of magically excluding files from the project based on the VERSIONS file, even if that's not actually what's happening. But I can understand that that's probably not intentional behaviour.

How does this work with other type checkers?

We run mypy via a custom Python script in CI. The script parses our VERSIONS file to determine which files to pass to mypy. Here's some of the logic that goes into that:

typeshed/tests/mypy_test.py

Lines 168 to 190 in 7df870f

	def parse_versions(fname: StrPath) -> dict[str, tuple[VersionTuple, VersionTuple]]:
	result: dict[str, tuple[VersionTuple, VersionTuple]] = {}
	with open(fname, encoding="UTF-8") as f:
	for line in f:
	line = strip_comments(line)
	if line == "":
	continue
	m = VERSION_LINE_RE.match(line)
	assert m, f"invalid VERSIONS line: {line}"
	mod: str = m.group(1)
	min_version = parse_version(m.group(2))
	max_version = parse_version(m.group(3)) if m.group(3) else (99, 99)
	result[mod] = min_version, max_version
	return result
	_VERSION_RE = re.compile(r"^([23])\.(\d+)$")
	def parse_version(v_str: str) -> tuple[int, int]:
	m = _VERSION_RE.match(v_str)
	assert m, f"invalid version: {v_str}"
	return int(m.group(1)), int(m.group(2))

I don't really want to move to a mypy_test.py-style approach for running pyright in CI; I much prefer being able to use pyright-action (it's a really nice user experience @jakebailey!). I'd be happy to move to an approach where we ran a short script to determine which files we need to exclude in CI (based on VERSIONS), and then passed those files as a command-line argument to pyright via pyright-action. Would that be possible, or can files to exclude only be specified via a config file?

[AlexWaygood]

I can't remember off the top of my head how we handle the VERSIONS file when running pytype in CI, but I think we do it in a similar way to mypy. We don't run pyre in CI at the moment (and never have in the past).

[erictraut]

@AlexWaygood, pyright already has some auto-exclude logic for typical projects. For example, it auto-excludes node_modules and **/__pycache__. It's easy to extend this logic to auto-exclude stdlib files that are "out of version" based on the VERSIONS file if the typeshedPath is .. I'm normally hesitant to add special-case hacks like this, but given the importance of typeshed, I think it's merited in this case. This will be included in the next release of pyright (1.1.312).

[AlexWaygood]

Thanks very much @erictraut, that's really helpful!

I'll mark this PR as deferred until the next pyright release.

[jakebailey]

I'm nearly done updating pyright-action to fix the previous bugs, but one thing I might acutally suggest if you're using virtual environments is to just activate the environment in a previous step and let PATH control what pyright does. That's closer to what people working on typeshed will be doing and means you don't have to pass the option for each pyright invocation.

Something like:

- run: echo "$PWD/.venv/bin" >> $GITHUB_PATH

Stuck right at the top would "activate" the venv for the further steps (and you could eliminate a whole bunch of source .venv/bin/activate lines in the workflows).

This is somewhat similar to what I suggest for poetry: jakebailey/pyright-action#10 (comment)

[AlexWaygood]

I'm nearly done updating pyright-action to fix the previous bugs, but one thing I might acutally suggest if you're using virtual environments is to just activate the environment in a previous step and let PATH control what pyright does. That's closer to what people working on typeshed will be doing and means you don't have to pass the option for each pyright invocation.

Something like:

- run: echo "$PWD/.venv/bin" >> $GITHUB_PATH

Stuck right at the top would "activate" the venv for the further steps (and you could eliminate a whole bunch of source .venv/bin/activate lines in the workflows).

This is somewhat similar to what I suggest for poetry: jakebailey/pyright-action#10 (comment)

@jakebailey nice, I would never have thought of that -- works great, thank you very much!

[jakebailey]

I just published pyright action 1.6.0, which should fix the --outputjson problem, the quoting problem, and also adds the shorthand for the new pyright flags (which I should have done earlier after reading release notes but forgot).

Of course, there's no need for any of those fixes in this PR anymore. 😄

[AlexWaygood]

@erictraut, it doesn't look like microsoft/pyright@2c97966 is having any impact on which files are excluded in our CI here. (We're now using the latest version of pyright in the CI for this PR.) Is there something else I need to do to take advantage of the new feature?

[erictraut]

You shouldn't need to do anything else to take advantage of the new feature. The fact that typeshedPath is set to . in the pyrightconfig.json file should be sufficient.

When I run pyright manually on typeshed (the latest main branch), I don't see the errors that are produced in CI. I'm using the same command that the github action is emitting, so I'd expect the behavior to be the same.

node <redacted>/pyright/packages/pyright/index.js --pythonplatform Darwin --pythonversion 3.12

My only theory is that it has to do with a difference in platforms. I'm running on a Mac, and the CI script is probably running on Linux. That means the CI script is using a case-sensitive file system. But I've reviewed my code, and I think I'm using normalized paths in my implementation, so case sensitivity shouldn't matter. If you happen to be using Linux locally, perhaps you could try running the above command-line and see what results you get.

[JelleZijlstra]

I tried on Linux, checking out this PR:

$ npx pyright  --pythonplatform Darwin --pythonversion 3.12
/home/jelle/repos/typeshed/stdlib/asynchat.pyi
  /home/jelle/repos/typeshed/stdlib/asynchat.pyi:1:8 - error: Stub file not found for "asyncore" (reportMissingTypeStubs)
/home/jelle/repos/typeshed/stdlib/smtpd.pyi
  /home/jelle/repos/typeshed/stdlib/smtpd.pyi:1:8 - error: Stub file not found for "asynchat" (reportMissingTypeStubs)
  /home/jelle/repos/typeshed/stdlib/smtpd.pyi:2:8 - error: Stub file not found for "asyncore" (reportMissingTypeStubs)
  /home/jelle/repos/typeshed/stdlib/smtpd.pyi:45:23 - error: "_MapType" is not a known member of module "asyncore" (reportGeneralTypeIssues)
  /home/jelle/repos/typeshed/stdlib/smtpd.pyi:75:23 - error: "_MapType" is not a known member of module "asyncore" (reportGeneralTypeIssues)

(And a lot more errors)

$ npx pyright --version
pyright 1.1.313

[erictraut]

I found the source of the problem. It was a bug in the feature I added. A subtle initialization ordering issue caused pyright to use the VERSIONS file in the packaged copy of typeshed rather than the typeshed stubs specified in typeshedPath. In the packaged typeshed, the VERSIONS file hadn't yet been updated to exclude asynchat and other deprecated modules from Python 3.12.

I convinced myself that the new feature was working fine in my local testing because I had a Python 3.11 venv selected, and it was able to resolve these deprecated modules (asynchat, etc.).

Apologies for the mix-up. I have a fix ready to check in, and I've done a bunch of testing to confirm that it works. I'll commit the change later today. The pylance team is actively working on their weekly sync with pyright, and I don't want to get in their way.

[erictraut]

Here's the pyright commit that addresses this. It will be included in next week's pyright release (1.1.314).

[AlexWaygood]

Thanks @erictraut and @jakebailey for your help here! Looks like everything's green on this PR with the latest pyright version.