Skip to content

Alternative fix for pyright CI issues #10121

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 1, 2023
Merged

Alternative fix for pyright CI issues #10121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,23 @@ jobs:
python-version: "3.10"
cache: pip
cache-dependency-path: requirements-tests.txt
- name: Install external dependencies for 3rd-party stubs
- 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: |
pip install -r requirements-tests.txt
DEPENDENCIES=$(python tests/get_external_stub_requirements.py)
if [ -n "$DEPENDENCIES" ]; then
source .venv/bin/activate
echo "Installing packages: $DEPENDENCIES"
pip install $DEPENDENCIES
fi
- run: pip freeze --all
- name: List 3rd-party stub dependencies installed
run: |
source .venv/bin/activate
pip freeze --all
- name: Get pyright version
uses: SebRollen/[email protected]
id: pyright_version
Expand All @@ -147,13 +155,15 @@ jobs:
python-platform: ${{ matrix.python-platform }}
python-version: ${{ matrix.python-version }}
no-comments: ${{ matrix.python-version != '3.10' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
venv-path: .
- 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.10' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
venv-path: .
project: ./pyrightconfig.stricter.json
- name: Run pyright on the test cases
uses: jakebailey/pyright-action@v1
Expand All @@ -162,6 +172,7 @@ jobs:
python-platform: ${{ matrix.python-platform }}
python-version: ${{ matrix.python-version }}
no-comments: ${{ matrix.python-version != '3.10' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
venv-path: .
project: ./pyrightconfig.testcases.json

stub-uploader:
Expand Down
1 change: 1 addition & 0 deletions pyrightconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"$schema": "https://github.com/raw/microsoft/pyright/main/packages/vscode-pyright/schemas/pyrightconfig.schema.json",
"typeshedPath": ".",
"venv": ".venv",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that it isn't going to work for people who prefer to name their venv something else? I usually name it env. It might not be a problem for me, because I don't run pyright locally very often.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it will mean that. FWIW, I also prefer env as a name for my local virtual environments (it's fewer characters to type), but went with .venv here since it's the name recommended by our CONTRIBUTING docs:

typeshed/CONTRIBUTING.md

Line 59 in ae0c9f9

$ python3 -m venv .venv

Copy link
Collaborator

Choose a reason for hiding this comment

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

.venv is also what's configured for flake8, gitignore, and isort.
mypy_test and regr_test use a temp directory anyway, but it's also using .venv

So it's pretty established that the standard for typeshed is to name the virtual environment folder .venv

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would still prefer passing it in a command-line option instead of hard-coding it in the config.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would still prefer passing it in a command-line option instead of hard-coding it in the config.

So would I, but if I'm reading the docs for pyright correctly, I don't think that's an option

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently pyright has a -v, --venv-path option whose purpose is exactly this. From the docs:

(3) This option is used in conjunction with configuration file, ... This allows a common config file to be checked in to the project and shared by everyone on the development team without making assumptions about the local paths to the venv directory on each developer’s computer.

Copy link
Member Author

@AlexWaygood AlexWaygood May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The venv-path option can be set via the command line (and I do in this PR, via pyright-action). But the venv-path option is meant to point to a directory in which there might be multiple different virtual environments, if I'm reading the pyright docs correctly, meaning it's useless to set the venv-path option on the command line unless you also set the venv option in the config file. The venv option is what tells pyright which venv it should select out of the ones it finds in the path specified by venv-path.

Copy link
Member Author

@AlexWaygood AlexWaygood May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

See the more detailed docs here, which the command-line docs link to, and which dig into the distinction between the venvPath and venv settings: https://github.com/microsoft/pyright/blob/main/docs/configuration.md#main-configuration-options

(venvPath in the config file is equivalent to venv-path on the command line)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently there's no way to set this on the command line.

"include": [
"stdlib",
"stubs",
Expand Down
1 change: 1 addition & 0 deletions pyrightconfig.stricter.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"$schema": "https://github.com/raw/microsoft/pyright/main/packages/vscode-pyright/schemas/pyrightconfig.schema.json",
"typeshedPath": ".",
"venv": ".venv",
"include": [
"stdlib",
"stubs",
Expand Down
1 change: 1 addition & 0 deletions pyrightconfig.testcases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"$schema": "https://github.com/raw/microsoft/pyright/main/packages/vscode-pyright/schemas/pyrightconfig.schema.json",
"typeshedPath": ".",
"venv": ".venv",
"include": [
"test_cases",
"stubs/**/@tests/test_cases"
Expand Down