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

Add .pylintrc Configuration File #17838

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

apivovarov
Copy link
Contributor

@apivovarov apivovarov commented Oct 1, 2024

This PR introduces a .pylintrc file to the XLA project.

XLA, which is used in TensorFlow, relies on TensorFlow's CI builds to automatically test its code, including Python components like xla_client. Recently, issues were encountered with TensorFlow's CI reporting errors such as pylint C0301: Line too long in xla_client_test.py.

TF CI links:

I found that XLA lacked a dedicated Pylint configuration (e.g., .pylintrc or pyproject.toml), which made it harder to automatically receive proper warnings and errors, particularly when working in editors like VSCode.

To address this, I have added a .pylintrc file to the XLA project. This file is based on TensorFlow's configuration, with modifications made to remove or fix unsupported configuration lines.

@apivovarov apivovarov requested a review from reedwm October 1, 2024 21:54
@reedwm
Copy link
Member

reedwm commented Oct 1, 2024

I don't know much about pylint. @ddunl can you review this?

@reedwm reedwm requested review from ddunl and removed request for reedwm October 1, 2024 22:19
@ddunl
Copy link
Member

ddunl commented Oct 1, 2024

I'll have to investigate what we do internally, an explicit goal I have for XLA is that the CI maps directly to what we do internally as often as possible (this is a work in progress, and sometimes is not possible). I'll have to see what we do internally. It's not totally clear to me that what TF does here is correct, because I don't think that on e.g. a PR to TF, that this pylint config is enforced in any meaningful way. I'll look into this and get back to you

@apivovarov
Copy link
Contributor Author

apivovarov commented Oct 1, 2024

JAX utilizes the pyproject.toml file, which includes the [tool.ruff.lint] section and other Ruff configurations. It no longer uses Pylint.

However, since the XLA development workflow is closely integrated with TensorFlow CI, we need to adhere to TensorFlow's Pylint rules. This approach helps us avoid spending valuable time resolving XLA PR merge issues reported by TensorFlow CI.

@ddunl
Copy link
Member

ddunl commented Oct 1, 2024

TF and JAX behave differently in this regard, TF may have that pylint job on PRs, but internally the status of the pylint on the PR is irrelevant - only the internal linter checks are respected. JAX does what I would prefer XLA do which is check both. I'm not exactly sure the mechanism by which JAX keeps the internal config in sync with the external one. One starting point I'd be interested in is to use https://google.github.io/styleguide/pylintrc and see if that currently agrees with all code we have checked in

@apivovarov
Copy link
Contributor Author

apivovarov commented Oct 1, 2024

TF pylintrc reports about 80 issues in xla/python

Styleguide pylintrc reports about 1000 issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants