Skip to content

CI: use github action for pyright #47067

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

Closed
wants to merge 1 commit into from
Closed

CI: use github action for pyright #47067

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
15 changes: 6 additions & 9 deletions .github/workflows/code-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,6 @@ jobs:
environment-file: ${{ env.ENV_FILE }}
use-only-tar-bz2: true

- name: Install node.js (for pyright)
uses: actions/setup-node@v3
with:
node-version: "16"

- name: Install pyright
# note: keep version in sync with .pre-commit-config.yaml
run: npm install -g [email protected]

- name: Build Pandas
id: build
uses: ./.github/actions/build_pandas
Expand All @@ -100,6 +91,12 @@ jobs:
run: ci/code_checks.sh typing
if: ${{ steps.build.outcome == 'success' }}

- uses: jakebailey/pyright-action@v1
with:
# note: keep version in sync with .pre-commit-config.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Curious, why do we need this in pre-commit-config and here i.e. can this just be run in pre-commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

pyright is only run by pre-commit during the manual stage. The idea is to give developers an easy way to run pyright without having to set it up (it depends on node). The CI doesn't run the manual stage.

I think the reason to keep mypy and pyright separate was that otherwise all the typing packages would need to be installed for the pre-commit hook. Maybe there was a better reason :)

Copy link
Member Author

Choose a reason for hiding this comment

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

the reason for not running pyright&mypy (by default) in pre-commit is that outdated/missing packages (e.g., numpy) would cause pyright&mypy to fail. We often have issues between numba requiring an older version of numpy but for typing we want the latest numpy.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Dr-Irv I think you set the pyright github action for https://github.com/pandas-dev/pandas-stubs up. How did you manage that pyright finds the virtual env? Running pyright directly (instead of using the github action) seemed to have no issue finding the virtual env.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense for typing checks in pre-commit to not necessarily run on every commit and therefore in manual mode. Is it possible to have the pre-commit job run these pyright checks then in the CI? My motivation is to see if there can be one clear way to run these checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be great to only use pre-commit and make mypy&pyright optional!

I think one issue with mypy+pre-commit is that pre-commit isolates the python environment (can probably be avoided by language: system)? Pyright doesn't seem to be affected by that since pre-commit doesn't seem to isolate non-python programs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr-Irv I think you set the pyright github action for https://github.com/pandas-dev/pandas-stubs up. How did you manage that pyright finds the virtual env? Running pyright directly (instead of using the github action) seemed to have no issue finding the virtual env.

I just copied the action from what Microsoft had set up in https://github.com/microsoft/python-type-stubs/blob/main/.github/workflows/test.yml . I think since it is a sequence of steps, the virtual env is automatically set up from the previous steps.

version: 1.1.247
extra-args: --skipunannotated

- name: Run docstring validation script tests
run: pytest scripts
if: ${{ steps.build.outcome == 'success' }}
Expand Down
7 changes: 0 additions & 7 deletions ci/code_checks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,6 @@ if [[ -z "$CHECK" || "$CHECK" == "typing" ]]; then
MSG='Performing static analysis using mypy' ; echo $MSG
mypy
RET=$(($RET + $?)) ; echo $MSG "DONE"

# run pyright, if it is installed
if command -v pyright &> /dev/null ; then
MSG='Performing static analysis using pyright' ; echo $MSG
pyright
RET=$(($RET + $?)) ; echo $MSG "DONE"
fi
fi

### SINGLE-PAGE DOCS ###
Expand Down
3 changes: 2 additions & 1 deletion pandas/_libs/missing.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ def is_matching_na(
) -> bool: ...
def isposinf_scalar(val: object) -> bool: ...
def isneginf_scalar(val: object) -> bool: ...
def checknull(val: object, inf_as_na: bool = ...) -> bool: ...
# test pyright + github action (reportInvalidStubStatement)
def checknull(val: object, inf_as_na: bool = True) -> bool: ...
def isnaobj(arr: np.ndarray, inf_as_na: bool = ...) -> npt.NDArray[np.bool_]: ...
def isnaobj2d(arr: np.ndarray, inf_as_na: bool = ...) -> npt.NDArray[np.bool_]: ...
def is_numeric_na(values: np.ndarray) -> npt.NDArray[np.bool_]: ...
Expand Down