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.

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

Disable pyright config? #9570

Merged
merged 8 commits into from
Oct 5, 2024
Merged

Conversation

max-sixty
Copy link
Collaborator

I'd propose disabling pyright in the pyproject.toml file for now, since including it means that all errors show up in devs' vscode, which then makes it more difficult to work with the actual errors. It overrides local VS Code settings, so I don't think is possible to disable...

I'd propose disabling pyright in the `pyproject.toml` file for now, since including it means that all errors show up in devs' vscode, which then makes it more difficult to work with the actual errors. It overrides local VS Code settings, so I don't think is possible to disable...
@Illviljan Illviljan added the run-pyright Run pyright type checker label Oct 2, 2024
@max-sixty
Copy link
Collaborator Author

The pyright tests fail in CI.

A few choices:

  • Current state — makes working in VS Code much noisier — does no one else find this? Or is there a workaround? I have > 50 errors, almost every file is marked red
  • Fix the pyright tests — very difficult, would be a tax to make both mypy & pyright pass
  • Ignore the errors inline — noisy, may conflict with mypy
  • Turn off the tests in CI — would be my suggestion. Folks can still enable them locally if they like. We don't get much value from them now, I don't see a realistic path to getting them fixed so we can get value from them
  • Add a config to ignore pyright, including locally.

What do folks think?

@headtr1ck
Copy link
Collaborator

I didn't check the details, but maybe one can add these options as command line options in CI instead?

@benbovy
Copy link
Member

benbovy commented Oct 3, 2024

How is the experience of Xarray contributors who use pyright as a LSP server but not from within vscode (pylance)?

Mine is pretty bad.

When running pyright version 1.1.349 on a recent commit in main with the following pyrightconfig.json:

{
    "useLibraryCodeForTypes": false,
    "exclude": ["./doc", "./asv_bench", "./properties", "./.eggs", "./xarray/util/generate_*.py", "./build"],
    "include": ["./xarray/**/*.py"],
    "ignore": ["./xarray/core/pycompat.py"],
    "reportMissingImports": "none",
    "reportMissingTypeStubs": false,
    "stubPath": "./xarray/core/",
    "typeCheckingMode": "basic",
    "strictParameterNoneValue": false,
    "analyzeUnannotatedFunctions": true,
    "reportGeneralTypeIssues": "information"
}

I get 1700 errors, 93 warnings, 41 informations.

Setting "analyzeUnannotatedFunctions": false I get 1015 errors, 82 warnings, 2527 informations .

If I exclude tests I get 335 errors, 73 warnings, 934 informations, which is still a lot.

I still use pyright as LSP server with emacs (+ native compilation and eglot) because it helps navigating / auto-completing the code, highlighting typing errors. I can hardly deal with the long list of diagnostics, though. And as soon as I have a few open buffers for large files like dataset.py or test_dataset.py, the LSP process constantly running in the background makes things laggy and my laptop fans are blowing as I type.

Am I configuring things wrong? Is there another LSP server that makes the overall experience much better (e.g., pylyzer, pylsp)? How does vscode (pylance) improves things? It would be a shame having to switch to vscode just for Xarray...
(cc @dcherian as I think you are also a emacs user)

Fixing all the pyright errors would be a huge task indeed. Disabling the tests (even locally) is not ideal since this is often where I catch typing issues or bugs before even running the tests. Ignoring pyright locally is not an option for me, unless there's another LSP server that provides a much better experience and that supports static type checking.

@max-sixty
Copy link
Collaborator Author

I still use pyright as LSP server with emacs (+ native compilation and eglot) because it helps navigating / auto-completing the code, highlighting typing errors.

Yes, in VS Code, pylance is extremely good at this, has made my life better!

@benbovy on this branch, do you still get all the errors? In my VS Code I get zero errors.

And for other VS-Coders, this is my config, super simple, zero errors reported on this branch. Notably changing typeCheckingMode to basic introduces some pyright errors (so I guess the default it to use mypy only and the config on main branch forces pyright to start showing errors.

  "python.analysis.autoImportCompletions": true,
  // "python.analysis.typeCheckingMode": "basic",
  "python.analysis.diagnosticSeverityOverrides": {
    // Xarray has try/expect-ed imports which we don't want to warn on; they're not
    // even installable on aarch64.
    "reportMissingImports": "information"
  },
  "python.analysis.exclude": ["build", ".trash", ".asv", ".venv", "target*"],
  "python.languageServer": "Pylance",

It would be a shame having to switch to vscode

No it wouldn't. (jk jk)


Who is most likely to object to the change? I strongly don't want to me interfering in others' work, especially since I'm a much less prolific maintainer these days. Would only make this change if it makes it better for others (and using my own experience as an example of others'...)

@benbovy
Copy link
Member

benbovy commented Oct 3, 2024

@max-sixty Yes I tried with the changes in this branch but I didn't notice any big difference.

(I'm wondering what pylance has on top of pyright to bring down the number of errors from hundreds (thousands) to zero!)

I don't have strong opinion about this PR... we can still customize the options locally in pyrightconfig.json, which I believe take precedence over the values defined in pyproject.toml

No it wouldn't.

Yes it would. 🙂

@max-sixty
Copy link
Collaborator Author

I don't have strong opinion about this PR... we can still customize the options locally in pyrightconfig.json, which I believe take precedence over the values defined in pyproject.toml

OK great!

Yes it would. 🙂

😀

@Illviljan
Copy link
Contributor

The pyright tests fail in CI.

A few choices:

* Current state — makes working in VS Code much noisier — does no one else find this? Or is there a workaround? I have > 50 errors, almost every file is marked red

* Fix the pyright tests — very difficult, would be a tax to make both mypy & pyright pass

* Ignore the errors inline — noisy, may conflict with mypy

* Turn off the tests in CI — would be my suggestion. Folks can still enable them locally if they like. We don't get much value from them now, I don't see a realistic path to getting them fixed so we can get value from them

* Add a config to ignore pyright, including locally.

What do folks think?

They're still useful unless we only care about the green color. You of course have to go and check the details and make sure you don't see anything unexpected.
The CI offers different python versions. At least I don't bother having that on my computer.

If you can move the options to the CI workflow that would be fine with me. When I added the CI I couldn't figure out how to do it, but I didn't investigate a lot nor do I use vscode so I didn't notice these issues.

@max-sixty
Copy link
Collaborator Author

Actually, sorry, this doesn't change anything about pyright passing or not — it always fails, on main and this branch. I just opened #9578 as a test.

Any objections to this being merged, then?

@benbovy
Copy link
Member

benbovy commented Oct 4, 2024

Am I configuring things wrong? Is there another LSP server that makes the overall experience much better (e.g., pylyzer, pylsp)?

Also cc @keewis who I think is not using vscode / pylance as well.

nor do I use vscode

@Illviljan -- What are you using?

(I'm genuinely curious about what developer tools (LSP) other Xarray developers are using, how they configure it and how their experience is better than mine).

(We should probable better continue discussing that in #5238).

@max-sixty
Copy link
Collaborator Author

Merging, hope that's OK, please feel free to revert if anyone has a strong view...

@max-sixty max-sixty merged commit 98596dd into pydata:main Oct 5, 2024
27 of 29 checks passed
@max-sixty max-sixty deleted the disable-pyright-config branch October 5, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-pyright Run pyright type checker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants