CI / tox: Update and speed up pyright #36443

mkoeppe · 2023-10-10T19:42:25Z

We bring tox -e pyright and the pyright invocation that is part of the CI "Build & Test" back in sync:

They are now both pinned to today's latest pyright version
node.js is provisioned via https://pypi.org/project/pyright/ and never accepts a preinstalled node version
node.js is configured to provide sufficient heap size. This reduces the runtime dramatically as pyright no longer has to fight with the garbage collector and eliminates spurious out of memory conditions.
Both run on the entire source tree.

This is done so that even if the output of pyright may be incomprehensible to developers, at least there are no gratuitous differences between what's shown on CI and what is shown locally.

Also done as part of this ticket: A limited effort to get rid of some warnings, just enough to establish that it would be premature to create problem matchers that annotate these warnings in the source code. Most of the warnings are meaningless and distracting noise because pyright is struggling with too many aspects of the Sage source code: lazy imports, methods in ParentMethods, ElementMethods, etc.

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

sage -tox -e pyright: Update, speed up, isolate #36515 (split out from here)
sage.schemes.toric: Remove pyright 'is possibly unbound' warnings #36516 (split out from here)
sage.misc.lazy_attribute: Add typestub file for pyright #36517 (split out from here)

kwankyu

Working fine.

mkoeppe · 2023-10-11T06:09:47Z

Thank you!

tobiasdiez

Sadly it doesn't really work. It now runs the check only on a subset of the files (the instructions in the tox file are obsolete).
Moreover, I don't think its a good idea to install a node package via an additional tour over tox + pip. I also would like to keep some version constraint to prevent that this test fails due some new releases. If you want to keep it up to date, add a simple rennovate config https://docs.renovatebot.com/modules/manager/regex/

mkoeppe · 2023-10-11T16:08:11Z

It now runs the check only on a subset of the files (the instructions in the tox file are obsolete).

Well, that this had fallen out of sync was a bug. Do we really want to run it on all files?

(Edit: Changed in 8bf3bcb)

mkoeppe · 2023-10-11T16:09:10Z

I also would like to keep some version constraint to prevent that this test fails due some new releases.

Please suggest a version constraint

(Edit: I've pinned it to what's current as of today in 81e1afa)

mkoeppe · 2023-10-11T16:58:02Z

I don't think its a good idea to install a node package via an additional tour over tox + pip.

It's better because we're reusing what is already configured, so it can't fall out of sync as it had.

outdated

tobiasdiez · 2023-10-13T10:24:06Z

I don't think its a good idea to install a node package via an additional tour over tox + pip.

It's better because we're reusing what is already configured, so it can't fall out of sync as it had.

Well, the tox version is still not working.

If you want to simplify the setup, you can use https://github.com/marketplace/actions/run-pyright. Also has the advantage that it registers annotation matchers.

mkoeppe · 2023-10-13T16:50:34Z

the tox version is still not working

Thanks, I see that.

…node installation

… 'sage -tox'

tobiasdiez · 2023-10-22T09:40:18Z

pyrightconfig.json

@@ -22,4 +22,7 @@
    "reportUndefinedVariable": "warning",
    "reportOptionalOperand": "warning",
    "reportMissingImports": "warning",
+    "reportPrivateImportUsage": "warning",
+    // Suppress because it flags our standard pattern @staticmethod __classcall__(cls, ...)


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)

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

tobiasdiez · 2023-10-22T09:42:54Z

.github/workflows/build.yml

@@ -103,20 +103,9 @@ jobs:
          MAKE: make -j2 --output-sync=recurse
          SAGE_NUM_THREADS: 2

-      - name: Set up node to install pyright


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

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.

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.

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.

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.

Beyond the scope of this PR.

tobiasdiez · 2023-10-22T09:43:58Z

pyrightconfig.json

@@ -22,4 +22,7 @@
    "reportUndefinedVariable": "warning",
    "reportOptionalOperand": "warning",
    "reportMissingImports": "warning",
+    "reportPrivateImportUsage": "warning",


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

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

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.

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.

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.

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

tobiasdiez · 2023-10-22T09:49:29Z

src/sage/misc/lazy_attribute.pyi

@@ -0,0 +1,3 @@
+# This type-stub file helps pyright understand the decorator @lazy_attribute.
+
+lazy_attribute = property


But this is not the correct type, right? lazy_attribute just happens to have the same typing as property (at the moment). In this case, just replace it by https://github.com/python/typeshed/blob/b9640005eb586afdbe0a57bac2b88a7a12465069/stdlib/builtins.pyi#L1237-L1254

Thanks, done in 05e34ef

github-actions · 2023-10-22T16:01:32Z

Documentation preview for this PR (built with commit 05e34ef; changes) is ready! 🎉

…nbound' warnings   Just a single commit split out from sagemath#36443 to facilitate review.    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36516 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee

mkoeppe · 2023-10-31T22:40:46Z

Closing as it has been replaced by #36515

…yright   Split out from sagemath#36443 to facilitate review.    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36517 Reported by: Matthias Köppe Reviewer(s): Tobias Diez

mkoeppe added the c: scripts label Oct 10, 2023

mkoeppe requested a review from tobiasdiez October 10, 2023 19:42

mkoeppe self-assigned this Oct 10, 2023

mkoeppe added the s: needs review label Oct 10, 2023

kwankyu approved these changes Oct 11, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Oct 11, 2023

tobiasdiez previously requested changes Oct 11, 2023

View reviewed changes

tobiasdiez added s: needs work and removed s: positive review labels Oct 11, 2023

mkoeppe added s: needs review and removed s: needs work labels Oct 11, 2023

mkoeppe requested a review from tobiasdiez October 11, 2023 16:58

tobiasdiez added s: needs work and removed s: needs review labels Oct 13, 2023

Matthias Koeppe added 5 commits October 21, 2023 19:13

src/tox.ini (pyright): Remove old version pin, do not rely on global …

bbc099f

…node installation

.github/workflows/build.yml: Simplify use of pyright by going through…

aa50168

… 'sage -tox'

src/tox.ini (pyright): Do not restrict to subdirectories by default

dce6b1f

src/tox.ini (pyright): Pin to 1.1.331 (latest)

02bd361

src/tox.ini (pyright): Pin to 1.1.332 (latest), update comment

ccf8a20

mkoeppe force-pushed the tox_pyright_update branch from 81e1afa to ccf8a20 Compare October 22, 2023 02:22

Matthias Koeppe added 2 commits October 21, 2023 19:47

src/tox.ini (pyright): Set memory limit to 8GB

8d686d6

src/sage/schemes/toric/variety.py: Fix pyright 'is possibly unbound'

f7dcfce

Matthias Koeppe added 4 commits October 21, 2023 21:06

pyrightconfig.json: Add comment

0dedbf9

pyrightconfig.json: Reduce reportPrivateImportUsage to warning

385a3e4

src/sage/misc/lazy_attribute.pyi: New

8129558

src/sage/misc/lazy_attribute.pyi: Add comment

8852022

mkoeppe added s: needs review and removed s: needs work labels Oct 22, 2023

tobiasdiez suggested changes Oct 22, 2023

View reviewed changes

tobiasdiez added s: needs work and removed s: needs review labels Oct 22, 2023

src/sage/misc/lazy_attribute.pyi: Proper type stub for lazy_attribute

05e34ef

mkoeppe added s: needs review and removed s: needs work labels Oct 22, 2023

mkoeppe changed the title ~~CI / tox: Update pyright~~ CI / tox: Update and speed up pyright Oct 22, 2023

tobiasdiez added s: needs work and removed s: needs review labels Oct 23, 2023

mkoeppe added s: needs review and removed s: needs work labels Oct 23, 2023

mkoeppe requested a review from tobiasdiez October 23, 2023 13:34

mkoeppe closed this Oct 31, 2023

github-actions bot removed the s: needs review label Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI / tox: Update and speed up pyright #36443

CI / tox: Update and speed up pyright #36443

mkoeppe commented Oct 10, 2023 •

edited

Loading

kwankyu left a comment

mkoeppe commented Oct 11, 2023

tobiasdiez left a comment

mkoeppe commented Oct 11, 2023 •

edited

Loading

mkoeppe commented Oct 11, 2023 •

edited

Loading

mkoeppe commented Oct 11, 2023

tobiasdiez commented Oct 13, 2023

mkoeppe commented Oct 13, 2023

tobiasdiez Oct 22, 2023

mkoeppe Oct 22, 2023

tobiasdiez Oct 22, 2023 •

edited

Loading

mkoeppe Oct 22, 2023

tobiasdiez Oct 22, 2023

mkoeppe Oct 22, 2023

tobiasdiez Oct 23, 2023

mkoeppe Oct 23, 2023

tobiasdiez Oct 22, 2023

mkoeppe Oct 22, 2023

tobiasdiez Oct 22, 2023

mkoeppe Oct 22, 2023

tobiasdiez Oct 23, 2023

mkoeppe Oct 23, 2023

tobiasdiez Oct 22, 2023

mkoeppe Oct 22, 2023

github-actions bot commented Oct 22, 2023

mkoeppe commented Oct 31, 2023

		@@ -0,0 +1,3 @@
		# This type-stub file helps pyright understand the decorator @lazy_attribute.

		lazy_attribute = property

CI / tox: Update and speed up pyright #36443

CI / tox: Update and speed up pyright #36443

Conversation

mkoeppe commented Oct 10, 2023 • edited Loading

📝 Checklist

⌛ Dependencies

kwankyu left a comment

Choose a reason for hiding this comment

mkoeppe commented Oct 11, 2023

tobiasdiez left a comment

Choose a reason for hiding this comment

mkoeppe commented Oct 11, 2023 • edited Loading

mkoeppe commented Oct 11, 2023 • edited Loading

mkoeppe commented Oct 11, 2023

tobiasdiez commented Oct 13, 2023

mkoeppe commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez Oct 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2023

mkoeppe commented Oct 31, 2023

mkoeppe commented Oct 10, 2023 •

edited

Loading

mkoeppe commented Oct 11, 2023 •

edited

Loading

mkoeppe commented Oct 11, 2023 •

edited

Loading

tobiasdiez Oct 22, 2023 •

edited

Loading