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

pylint partioning should be optional #21686

Open
purajit opened this issue Nov 23, 2024 · 1 comment
Open

pylint partioning should be optional #21686

purajit opened this issue Nov 23, 2024 · 1 comment
Labels
backend: Python Python backend-related issues bug

Comments

@purajit
Copy link
Contributor

purajit commented Nov 23, 2024

Describe the bug
The issue is that partitioning the files before running pylint on each batch is a fundamentally incorrect way to run pylint, since there are checkers whose inference relies on complete file traversal. For example, with the follow file system:

# a.py
from .b import *

# b.py
from .c import *

# c.py
from .a import *
$ pylint --score=no --disable=W0401 a.py c.py

$ pylint --score=no --disable=W0401 a.py b.py

$ pylint --score=no --disable=W0401 c.py b.py

$ pylint --score=no --disable=W0401 .
************* Module pylintpartialtest.b
b.py:1:0: R0401: Cyclic import (pylintpartialtest.a -> pylintpartialtest.b -> pylintpartialtest.c) (cyclic-import)

Pylint doesn't do a great job of making this obvious in the docs, but there's a mention here:

Be aware, though: pylint should analyze all your code at once in order to best infer the actual values that result from calls. If only some of the files are given, pylint might miss a particular value's type and produce inferior inference for the subset. It can also be unexpectedly different when the file set changes because the new slicing can change the inference. So, don't do this if correctness and determinism are important to you.

I discovered this because we've been reducing our dependency on Pants, and when I ran a full pylint on our repo, I discovered 1000+ failing checks that were never caught in the few years we've been running Pants.

IMO this should be an explicit configuration that defaults to not partitioning, since that's the expected way to pylint, and prevent the default run from hiding issues. If people want to speed up their runs by partitioning they should explicitly choose this knowing what the consequences are (for instance, you may run a faster version in CI and the complete version on trunk). At the very least, an option to run it without partitioning, especially when using ::, should be provided.

Pants version
All, we started very early in v2, currently on v2.24dev

OS
MacOS and Linux

@purajit purajit added the bug label Nov 23, 2024
@huonw huonw added the backend: Python Python backend-related issues label Nov 25, 2024
@huonw
Copy link
Contributor

huonw commented Nov 25, 2024

Thanks for flagging.

This is conceptually related to #15069 and #18410, although a solution that works for them (e.g. create 'dummy' files since those only care about file existence) may not work for pylint, that actually needs to read the contents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues bug
Projects
None yet
Development

No branches or pull requests

2 participants