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.

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

CI / tox: Update and speed up pyright #36443

Closed
wants to merge 12 commits into from
Closed

CI / tox: Update and speed up pyright #36443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bbc099f
src/tox.ini (pyright): Remove old version pin, do not rely on global …
mkoeppe Oct 10, 2023
aa50168
.github/workflows/build.yml: Simplify use of pyright by going through…
mkoeppe Oct 10, 2023
dce6b1f
src/tox.ini (pyright): Do not restrict to subdirectories by default
mkoeppe Oct 11, 2023
02bd361
src/tox.ini (pyright): Pin to 1.1.331 (latest)
mkoeppe Oct 11, 2023
ccf8a20
src/tox.ini (pyright): Pin to 1.1.332 (latest), update comment
mkoeppe Oct 22, 2023
8d686d6
src/tox.ini (pyright): Set memory limit to 8GB
mkoeppe Oct 22, 2023
f7dcfce
src/sage/schemes/toric/variety.py: Fix pyright 'is possibly unbound'
mkoeppe Oct 22, 2023
0dedbf9
pyrightconfig.json: Add comment
mkoeppe Oct 22, 2023
385a3e4
pyrightconfig.json: Reduce reportPrivateImportUsage to warning
mkoeppe Oct 22, 2023
8129558
src/sage/misc/lazy_attribute.pyi: New
mkoeppe Oct 22, 2023
8852022
src/sage/misc/lazy_attribute.pyi: Add comment
mkoeppe Oct 22, 2023
05e34ef
src/sage/misc/lazy_attribute.pyi: Proper type stub for lazy_attribute
mkoeppe Oct 22, 2023
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
13 changes: 1 addition & 12 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,20 +103,9 @@ jobs:
MAKE: make -j2 --output-sync=recurse
SAGE_NUM_THREADS: 2

- name: Set up node to install pyright
Copy link
Contributor

@tobiasdiez tobiasdiez Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I still think that this way of installing pyright is better. I also don't get the consistency argument (as it comes down to calling pyright without any other arguments). Tox just installs (and calls) pyright in a very convoluted way. And this is exemplified by the memory issues. What to you think about using https://github.com/jakebailey/pyright-action? Also has the advantage of adding problem matchers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I [...] don't get the consistency argument (as it comes down to calling pyright without any other arguments).

It comes down to not only that, but also to running the same pyright version, on the same Node version.

Tox just installs (and calls) pyright in a very convoluted way. And this is exemplified by the memory issues.

There's nothing convoluted about it. This is how one installs and runs pyright using Python infrastructure without having to rely on a system installation of Node.

And there is nothing mysterious about "memory issues", it is a default heap size of 2 GB, which is too small for our purpose. Here I configure it explicitly so that proper function does not depend on an externally set configuration.

What to you think about using https://github.com/jakebailey/pyright-action? Also has the advantage of adding problem matchers

I looked at it, as you had mentioned it before. As I explained in the PR description, adding Check Warnings for pyright's complaints just makes no sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's nothing convoluted about it. This is how one installs and runs pyright using Python infrastructure without having to rely on a system installation of Node.

Yes, but we have a node environment here, so no need to use the Python infrastructure. I'm aware that its more convenient for the average Python dev to run it that way locally. But that doesn't mean the ci needs to do that way, and in fact the node-setup action is the githuby way of installing node in a github workflow. So please use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in fact the node-setup action is the githuby way of installing node in a github workflow.

No, there's not only "one way" of doing things right.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also easily disable comments for the pyright action if that's your concern. I would say we should run it twice: once without commenting for all functions, and a second time with comments on type-annotated functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beyond the scope of this PR.

if: always() && steps.worktree.outcome == 'success'
uses: actions/setup-node@v3
with:
node-version: '12'

- name: Install pyright
if: always() && steps.worktree.outcome == 'success'
# Fix to v232 due to bug https://github.com/microsoft/pyright/issues/3239
run: npm install -g [email protected]

- name: Static code check with pyright
if: always() && steps.worktree.outcome == 'success'
run: pyright
run: ./sage -tox -e pyright
working-directory: ./worktree-image

- name: Clean (fallback to non-incremental)
Expand Down
3 changes: 3 additions & 0 deletions pyrightconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@
"reportUndefinedVariable": "warning",
"reportOptionalOperand": "warning",
"reportMissingImports": "warning",
"reportPrivateImportUsage": "warning",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fixing the single instance of this error (by properly importing the offending method)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see the immediate fix, and it did not seem worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember the names, but sage is importing x from y, and y is importing x from z (without reexporting it). So just import x from z.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't even have such a thing as a convention of what reexports should look like in Sage. So warning instead of error is quite appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here y is an external library. So sage was accessing a non-public api. I think this is a good catch and should be fixed properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't plan to work on this import. If you want to fix stuff, just open a PR.

// Suppress because it flags our standard pattern @staticmethod __classcall__(cls, ...)
Copy link
Contributor

Choose a reason for hiding this comment

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

comments in json files are sadly not possible. the common workaround I've seen is to add them as another field, i.e. reportSelfClsParameterName.comment (not sure if it works here or if pyright complains)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a Microsoft extension ("JSONC"), see https://github.com/microsoft/node-jsonc-parser

"reportSelfClsParameterName": "none",
}
9 changes: 9 additions & 0 deletions src/sage/misc/lazy_attribute.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# This type-stub file helps pyright understand the decorator @lazy_attribute.

# Adapted from https://github.com/python/typeshed/blob/b9640005eb586afdbe0a57bac2b88a7a12465069/stdlib/builtins.pyi#L1237-L1254
class lazy_attribute:
def __init__(
self,
f: Callable[[Any], Any] | None = ...
) -> None: ...
def __get__(self, a: Any, cls: type) -> Any: ...
25 changes: 13 additions & 12 deletions src/sage/schemes/toric/variety.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2250,20 +2250,21 @@ def Todd_class(self, deg=None):
1
"""
Td = QQ.one()
if self.dimension() >= 1:
dim = self.dimension()
if dim >= 1:
c1 = self.Chern_class(1)
Td += QQ.one() / 2 * c1
if self.dimension() >= 2:
c2 = self.Chern_class(2)
Td += QQ.one() / 12 * (c1**2 + c2)
if self.dimension() >= 3:
Td += QQ.one() / 24 * c1*c2
if self.dimension() >= 4:
c3 = self.Chern_class(3)
c4 = self.Chern_class(4)
Td += -QQ.one() / 720 * (c1**4 - 4*c1**2*c2 - 3*c2**2 - c1*c3 + c4)
if self.dimension() >= 5:
raise NotImplementedError('Todd class is currently only implemented up to degree 4')
if dim >= 2:
c2 = self.Chern_class(2)
Td += QQ.one() / 12 * (c1**2 + c2)
if dim >= 3:
Td += QQ.one() / 24 * c1*c2
if dim >= 4:
c3 = self.Chern_class(3)
c4 = self.Chern_class(4)
Td += -QQ.one() / 720 * (c1**4 - 4*c1**2*c2 - 3*c2**2 - c1*c3 + c4)
if dim >= 5:
raise NotImplementedError('Todd class is currently only implemented up to degree 4')
if deg is None:
return Td
else:
Expand Down
15 changes: 8 additions & 7 deletions src/tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,22 @@ commands =
[testenv:pyright]
description =
run the static typing checker pyright
deps = pyright
deps =
pyright
setenv =
{[sagedirect]setenv}
HOME={envdir}
# Fix version, see .github/workflows/build.yml
PYRIGHT_PYTHON_FORCE_VERSION=1.1.232
PYRIGHT_PYTHON_FORCE_VERSION=1.1.332
PYRIGHT_PYTHON_GLOBAL_NODE=false
NODE_OPTIONS=--max-old-space-size=8192
allowlist_externals = {[sagedirect]allowlist_externals}
## We run pyright from within the sage-env so that SAGE_LOCAL is available.
## pyright is already configured via SAGE_ROOT/pyrightconfig.json to use our venv.
##
## Running pyright on the whole Sage source tree takes very long
## and may run out of memory. When no files/directories are given, just run it
## on the packages that already have typing annotations.
## Note that running pyright on the whole Sage source tree (= default) takes very long
## and may run out of memory.
commands =
{env:SAGE} -sh -c 'pyright {posargs:{toxinidir}/sage/combinat {toxinidir}/sage/manifolds}'
{env:SAGE} -sh -c 'pyright {posargs:}'

[testenv:pycodestyle]
description =
Expand Down