Skip to content

Conversation

@adam-smnk
Copy link
Contributor

@adam-smnk adam-smnk commented Nov 20, 2025

Adds 'ruff' as Python linter and code formatter, and 'pre-commit' for easy rules application both locally and in CI.
New lint CI workflow is also added.

The formatting tools are added to the 'dev' dependency group.
They are installed by default with uv sync and can be run locally with:
pre-commit run -a

@adam-smnk
Copy link
Contributor Author

The PR is split into two commits to make reviewing simpler.
The first one contains only the new linter infrastructure.
The second commit applies formatting to the repo.

Current lint config is a simple opinionated collection of rules.
Open to suggestions and changes. 🙂

@rengolin
Copy link
Member

If a PR fails on formatting, is there an actionable thing for developers to do to fix the formatting without having to patch by hand every difference?

@rolfmorel
Copy link
Contributor

If a PR fails on formatting, is there an actionable thing for developers to do to fix the formatting without having to patch by hand every difference?

Would be great to set it up as on llvm-project, e.g. llvm/llvm-project#146732 (comment) where a Github action comments something like

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r HEAD~1...HEAD mlir/python/mlir/dialects/transform/tune.py 
mlir/test/python/dialects/transform_tune_ext.py

View the diff from darker here.
--- python/mlir/dialects/transform/tune.py	2025-07-03 14:02:19.000000 +0000
+++ python/mlir/dialects/transform/tune.py	2025-07-03 14:05:57.797604 +0000
@@ -28,11 +28,11 @@
 
 @_ods_cext.register_operation(_Dialect, replace=True)
 class KnobOp(KnobOp):
     def __init__(
         self,
-        result: Type, # !transform.any_param or !transform.param<Type>
+        result: Type,  # !transform.any_param or !transform.param<Type>
         name: Union[StringAttr, str],
         options: Union[
             ArrayAttr, Sequence[Union[Attribute, bool, int, float, str]], Attribute
         ],
         *,
@@ -67,11 +67,11 @@
             ip=ip,
         )
 
 
 def knob(
-    result: Type, # !transform.any_param or !transform.param<Type>
+    result: Type,  # !transform.any_param or !transform.param<Type>
     name: Union[StringAttr, str],
     options: Union[
         ArrayAttr, Sequence[Union[Attribute, bool, int, float, str]], Attribute
     ],
     *,

and then on further revisions the comment is edited to become:

✅ With the latest revision this PR passed the Python code formatter.

Just need to find the right Github workflows to copy (and appropriately massage) from llvm-project/.github.

@rolfmorel
Copy link
Contributor

rolfmorel commented Nov 23, 2025

On the topic of copying from llvm-project: I would prefer if we also just keep to whichever code style is being enforced upstream. Just need to dig around a bit to find the right files that specify it.

@adam-smnk
Copy link
Contributor Author

If a PR fails on formatting, is there an actionable thing for developers to do to fix the formatting without having to patch by hand every difference?

Most cases can be addressed automatically by running pre-commit run --all-files locally (no need for copy-pasting). It applies formatting and "safe" fixes (docs).
During CI run, job will still fail (no magic fixes from CI) but a diagnostic message is reported, the below example indicates that all issues can be fixed automatically:

ruff check...............................................................Failed
- hook id: ruff-check
- files were modified by this hook

Found 1 error (1 fixed, 0 remaining).

ruff format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

1 file reformatted, 19 files left unchanged

In case of "unsafe" fixes, they have to be addressed manually but errors are reported and, when possible, hints are provided:

ruff check...............................................................Failed
- hook id: ruff-check
- exit code: 1

D419 Docstring is empty
 --> my_file.py:2:5
  |
1 | def test():
2 |     """"""
  |     ^^^^^^
3 |     return
  |

F401 `.runtime_args.get_packed_arg` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
 --> python/lighthouse/utils/__init__.py:3:27
  |
1 | """A collection of utility tools"""
2 |
3 | from .runtime_args import get_packed_arg
  |                           ^^^^^^^^^^^^^^
  |
help: Use an explicit re-export: `get_packed_arg as get_packed_arg`

Found 2 errors.

ruff format..............................................................Passed

Example of failing CI from my fork [link].

@adam-smnk
Copy link
Contributor Author

where a Github action comments

Do we really need comments spam?
Failing CI job should be indicative enough that there's a problem and job logs provide further info if needed.

On the topic of copying from llvm-project: I would prefer if we also just keep to whichever code style is being enforced upstream. Just need to dig around a bit to find the right files that specify it.

Makes sense. I'll have a look how it's configured.

@adam-smnk adam-smnk force-pushed the ci-lint branch 2 times, most recently from 9fd08a3 to 554b268 Compare November 24, 2025 08:39
@rengolin
Copy link
Member

Do we really need comments spam?
Failing CI job should be indicative enough that there's a problem and job logs provide further info if needed.

If the failing CI has a clear message on how to reproduce locally and fix, then we don't need additional comments.

@rengolin
Copy link
Member

You need to rebase your branch to run the two new workflows from main

@adam-smnk
Copy link
Contributor Author

You need to rebase your branch to run the two new workflows from main

It is already rebased

@adam-smnk
Copy link
Contributor Author

On the topic of comparison to llvm-project setup.
AFAIK, there's only Python formatter darker, which applies Black by default, and there's no Python linter.

Ruff formatter is largely aligned with Black with minor deviations.
Ruff linter's isort is also intended to be compatible with Black. The currently set two non-default isort rules didn't trigger any reshuffling by black when I run it on top of this PR.

I'd suggest we keep lint to aim for higher code quality. Exact rules can be refined over time.
For formatting, ruff should keep us largely aligned with llvm-project. Anecdotally, ruff seems about 2x faster than black. However, we could switch to using black for formatting, instead.

@rengolin
Copy link
Member

You need to rebase your branch to run the two new workflows from main

It is already rebased

Ah, my fault! I have changed the branch protection expecting #22 to have been merged yesterday. I have reverted back to the original test until #24 is merged.

@rengolin
Copy link
Member

Ok, now #22 is merged, we can rebase.

@adam-smnk
Copy link
Contributor Author

adam-smnk commented Nov 24, 2025

Reshuffled and amended commits to keep reviewing simpler.

Most recent change: dropped non-default isort rules around import order and formatting. Default are less invasive and more in line with llvm-project i.e., no particular import style is enforced. Simple alphabetical ordering is still performed within single-line imports.

Edit: reformatted again but preserved custom args comment layout in mlp_from_file.

Adds 'ruff' as Python linter and code formatter and 'pre-commit' for
easy rules application both locally and in CI.
New lint CI workflow is also added.

The formatting tools are added as optional dependencies.
They can be installed using:
  uv sync --extra dev-tools
@adam-smnk
Copy link
Contributor Author

adam-smnk commented Nov 25, 2025

Tweaked rules and rerun formatter after more experimentation and offline discussions.

Changes:

  • disabled isort - ruff doesn't expose all controls and it makes import formatting too opinionated; there's value in standardized layout but I'll leave this discussion for another time
  • added a few extra RUF linter rules
  • reapplied formatting - now the change diff is smaller, imports are no longer formatted

Copy link
Member

@rengolin rengolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with CI fixed.

@adam-smnk adam-smnk merged commit 7e92c48 into llvm:main Nov 25, 2025
3 checks passed
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