Remove pylint #11183

harupy · 2024-02-19T13:05:17Z

🛠 DevTools 🛠

Install mlflow from this PR

pip install git+https://github.com/mlflow/mlflow.git@refs/pull/11183/merge

Checkout with GitHub CLI

gh pr checkout 11183

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Remove pylint because it's very slow and we have ruff.
The custom rules (no-rst and lazy-builtin-import) have been migrated to a simple linter using the ast module, which is much faster than pylint.

Follow-up tasks

Remove pylint disable comments.

How is this PR tested?

Existing unit/integration tests
New unit/integration tests
Manual tests

Does this PR require documentation update?

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

Interface

area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
area/windows: Windows support

Language

language/r: R APIs and clients
language/java: Java APIs and clients
language/new: Proposals for new client languages

Integrations

integrations/azure: Azure and Azure ML integrations
integrations/sagemaker: SageMaker integrations
integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

github-actions · 2024-02-19T13:05:40Z

Documentation preview for a6dcdba will be available when this CircleCI job
completes successfully.

More info

Ignore this comment if this PR does not change the documentation.
It takes a few minutes for the preview to be available.
The preview is updated when a new commit is pushed to this PR.
This comment was created by https://github.com/mlflow/mlflow/actions/runs/7970870409.

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy · 2024-02-20T00:44:39Z

dev/clint/pyproject.toml

Linting all files takes 0.6 ~ 0.7 sec on my machine:

% time clint $(git ls-files '*.py') clint $(git ls-files '*.py') 4.93s user 0.59s system 815% cpu 0.677 total % time clint $(git ls-files '*.py') clint $(git ls-files '*.py') 4.75s user 0.59s system 823% cpu 0.649 total % time clint $(git ls-files '*.py') clint $(git ls-files '*.py') 4.70s user 0.58s system 805% cpu 0.655 total

pylint

% time pylint $(git ls-files '*.py') -------------------------------------------------------------------- Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00) pylint $(git ls-files '*.py') 192.85s user 38.47s system 794% cpu 29.106 total

THIS IS LIFE CHANGING!!

harupy · 2024-02-20T02:19:58Z

dev/clint/README.md

+# Integrating with Visual Studio Code
+
+1. Install [the Pylint extension](https://marketplace.visualstudio.com/items?itemName=ms-python.pylint)
+2. Add the following setting in your `settings.json` file:
+
+```json
+{
+  "pylint.path": ["${interpreter}", "-m", "clint"]
+}
+```


clint behaves like pylint and can be used with the pylint extension.

harupy · 2024-02-20T05:42:49Z

.github/workflows/lint.yml

          echo "::add-matcher::.github/workflows/matchers/format.json"
          echo "::add-matcher::.github/workflows/matchers/ruff.json"
      - uses: ./.github/actions/cache-pip
      - name: Install dependencies
        run: |
          python -m venv .venv
          source .venv/bin/activate
-          source ./dev/install-common-deps.sh --ml


Pylint also checks class inheritance, so we needed to install ML packages mlflow depends on.

B-Step62

The result looks truly amazing!! Overall looks good to me, just added a few minor commnets.

B-Step62 · 2024-02-20T07:44:12Z

.pre-commit-config.yaml

-      - id: disallow-rule-ids
-        name: disallow-rule-ids
-        entry: python ./dev/disallow_rule_ids.py
+      - id: clint


Can we name the step a bit verbosely like "custom-python-lint", so that external contributors can easily tell what this step is?

sounds good!

B-Step62 · 2024-02-20T07:45:16Z

dev/clint/README.md

@@ -0,0 +1,22 @@
+# Installation


nit: a quick description of what this linter is for will be helpful

B-Step62 · 2024-02-20T07:46:04Z

dev/clint/pyproject.toml

THIS IS LIFE CHANGING!!

B-Step62 · 2024-02-20T07:54:12Z

dev/clint/src/clint/linter.py

+    def __str__(self):
+        return f"{self.path}:{self.lineno}:{self.col_offset}: {self.rule.id}: {self.rule.message}"
+
+    def json(self) -> dict[str, str | int | None]:


What is this JSON serialization used for? Perhaps VSCode integration? (just curious about the difference from pylint plugins' implementation)

It's used by the pylint extension :) The pylint extension (written in typescript) runs pylint in a subprocess and parse stdout as JSON to collect lint errors.

B-Step62 · 2024-02-20T08:10:43Z

dev/clint/src/clint/linter.py

+
+    def visit_ClassDef(self, node: ast.ClassDef) -> None:
+        self.stack.append(node)
+        self._no_rst(node)


This is more about future extension, but is it possible to define those rules pluggable like pylint (or perhaps declarative)? Currently when we want to add/remove rules for clint, we need to directly edit the single node traversal class, which might be a bit error-prone. Of course it doesn't need to be in this PR but sth we can think of in long-term.

It's possible to define a visitor for each rule like pylint but it has a con. That's slow because each visitor needs to traverse the code. I'd consider making this pluggable if we had more (> 5) rules.

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> Signed-off-by: Arthur Jenoudet <arthur.jenoudet@databricks.com>

harupy added 9 commits February 19, 2024 20:09

Remove pylint

e709a10

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Fix config

6815cc6

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Lint setup.py

cca0867

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

8ee0405

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

28b0977

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

5a7e60e

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

5504a49

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

a09828b

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

95d41e4

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

github-actions bot added the rn/none List under Small Changes in Changelogs. label Feb 19, 2024

harupy added 14 commits February 19, 2024 22:05

wip

0d19be4

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

98089c2

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

wip

bcd71c0

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

readme

4e0301f

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

fix __str__

f144278

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

fix __str__

25b4742

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

None

0eba5f2

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

disable

aa51415

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Remove disallow_rule_ids

d816d7a

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Improve README

d891ab1

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Installation

cb2abc5

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

add zero

c0e8890

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

TODO

f7f030f

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

simplify if

7a52c1d

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy changed the title ~~[WIP] Remove pylint~~ Remove pylint Feb 20, 2024

Do not install mlflow

7253e9f

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy commented Feb 20, 2024

View reviewed changes

harupy requested a review from mlflow-automation February 20, 2024 01:57

mlflow-automation requested a review from B-Step62 February 20, 2024 01:57

mlflow-automation requested review from BenWilson2, daniellok-db, serena-ruan and WeichenXu123 and removed request for mlflow-automation February 20, 2024 01:57

harupy commented Feb 20, 2024

View reviewed changes

B-Step62 approved these changes Feb 20, 2024

View reviewed changes

Address comments

a6dcdba

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy merged commit 8915365 into mlflow:master Feb 20, 2024
41 checks passed

harupy deleted the remove-pylint branch February 20, 2024 10:55

harupy mentioned this pull request Feb 20, 2024

Remove pylint: disable comments #11196

Merged

37 tasks

sateeshmannar pushed a commit to StateFarmIns/mlflow that referenced this pull request Feb 20, 2024

Remove pylint (mlflow#11183)

89b3de4

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

artjen pushed a commit to artjen/mlflow that referenced this pull request Mar 26, 2024

Remove pylint (mlflow#11183)

6048836

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> Signed-off-by: Arthur Jenoudet <arthur.jenoudet@databricks.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pylint #11183

Remove pylint #11183

harupy commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

harupy Feb 20, 2024 •

edited

harupy Feb 20, 2024

B-Step62 Feb 20, 2024

harupy Feb 20, 2024 •

edited

harupy Feb 20, 2024

B-Step62 left a comment

B-Step62 Feb 20, 2024 •

edited

harupy Feb 20, 2024

B-Step62 Feb 20, 2024

harupy Feb 20, 2024

B-Step62 Feb 20, 2024

B-Step62 Feb 20, 2024

harupy Feb 20, 2024 •

edited

B-Step62 Feb 20, 2024

harupy Feb 20, 2024 •

edited

Remove pylint #11183

Remove pylint #11183

Conversation

harupy commented Feb 19, 2024 • edited

Install mlflow from this PR

Checkout with GitHub CLI

Related Issues/PRs

What changes are proposed in this pull request?

Follow-up tasks

How is this PR tested?

Does this PR require documentation update?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

github-actions bot commented Feb 19, 2024 • edited

harupy Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B-Step62 left a comment

Choose a reason for hiding this comment

B-Step62 Feb 20, 2024 • edited

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

harupy Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Feb 20, 2024 • edited

Choose a reason for hiding this comment

harupy commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024 •

edited

harupy Feb 20, 2024 •

edited

harupy Feb 20, 2024 •

edited

B-Step62 Feb 20, 2024 •

edited

harupy Feb 20, 2024 •

edited

harupy Feb 20, 2024 •

edited