Python tidy and formatter #1978

dorde-antic · 2025-09-08T11:37:57Z

Motivation

This PR introduces tooling to enforce consistent Python formatting and linting within the rocMLIR repository. Resolves https://github.com/ROCm/rocMLIR-internal/issues/1872/

Technical Details

Added .github/workflows/ci.yml to implement a GitHub Actions workflow that runs on develop for both pushes and pull requests. The workflow installs flake8, yapf, and pep8-naming, then runs:

flake8, excluding the external/ directory
yapf --dry-run, excluding external/**/*.py, and reporting format issues

Added .pre-commit-config.yaml to configure local pre-commit hooks with:

YAPF (auto-formatting) using -i
Flake8 (with pep8-naming plugin)
An exclude rule to ignore files under external/

Added .style.yapf to define YAPF formatting preferences.

Test Plan

1. Github Actions CI Testing

Temporarily adding different types python files to test if they trigger Github Actions and if the CI works properly.
I'll try more cases with different py files (some that should pass checks and some that should fail)

Warning

Do not merge with python files from this PR since they are used for testing

2. Local Testing

Run pre-commit install, followed by pre-commit run --all-files to confirm YAPF and Flake8 are working correctly.
Attempt to commit changes (with or without dummy files), and verify that the pre-commit hooks execute upon commit.

3. Github Actions CI + pre-commit

Confirm that if pre-commit made changes, it made them in a way that they will pass the Github Actions check.

Test Result

In progress ⏳

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

codecov · 2025-09-08T14:41:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1978      +/-   ##
===========================================
+ Coverage    79.50%   79.62%   +0.11%     
===========================================
  Files          100      102       +2     
  Lines        31016    31910     +894     
  Branches      4819     5047     +228     
===========================================
+ Hits         24659    25406     +747     
- Misses        4245     4312      +67     
- Partials      2112     2192      +80

Flag	Coverage Δ
mfma	`?`
navi4x	`79.62% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.
see 50 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR introduces Python code formatting and linting infrastructure for the rocMLIR repository, implementing both CI automation and local development tooling to enforce consistent Python code style.

Adds GitHub Actions CI workflow for automated Python code quality checks on pull requests and pushes
Configures pre-commit hooks with YAPF formatter and Flake8 linter for local development
Sets up YAPF style configuration based on PEP8 with project-specific customizations

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pip_requirements.txt	Adds Python formatting and linting dependencies
.github/workflows/ci.yml	Implements CI workflow for Python code quality checks
.pre-commit-config.yaml	Configures local pre-commit hooks for YAPF and Flake8
.style.yapf	Defines YAPF formatting style configuration
mlir/utils/performance/dummyfile.py	Test file with intentional formatting issues for CI validation

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pip_requirements.txt

mlir/utils/performance/dummyfile.py

dorde-antic · 2025-09-09T19:31:55Z

We might want to consider making Python Lint and Format Check a required check for PRs (once it's tested on different cases so that it won't block PRs from merging for wrong reason)
We should thing about defining a naming convention for Python (and other related stuff) across the project, so that we adjust settings for flake and yapf before merging

Atm this is how CI (Github Action) look like in case of current py file:

and this is example how it looks for other files we already have inside mlir dir (atm with current settings):

Co-authored-by: Copilot <[email protected]>

dhernandez0 · 2025-09-10T08:06:24Z

please, can you reformat all existing python files in this PR?

dorde-antic · 2025-09-10T12:14:29Z

please, can you reformat all existing python files in this PR?

i can, btw, changes introduced in this PR would make Github action fail only if files that are changed in future PRs are python and inside mlir dir. So it wouldn't fail because of all other existing files are unformatted. The screenshot above that shows all python files in mlir/ ... was testing to see how it would look (when tried on all py files),

but for example for this PR Github Actions CI would only show this:

and not all of the py files in mlir/

umangyadav · 2025-09-10T12:20:48Z

.github/workflows/ci.yml

+
+on:
+  pull_request:
+    branches: [develop]


Add release branches too ?

@umangyadav one check, it would be all from release/* ?

.pre-commit-config.yaml

umangyadav · 2025-09-10T12:25:42Z

.pre-commit-config.yaml

+exclude: '^external/.*'
+
+repos:
+  - repo: https://github.com/google/yapf


Because it is pre-commit hook, does it mean we need to update our docker locally for this to work ? You may want to notify team about this once this is merged.

dorde-antic · 2025-09-13T15:13:58Z

And this is example when YAPF check fails (happened after switching to google style):

.github/workflows/ci.yml

umangyadav · 2025-09-19T12:43:04Z

.style.yapf

@@ -0,0 +1,4 @@
+[style]
+  based_on_style = google
+  column_limit = 79


why 79? . it's an odd number.

it's flake8's default, but we can change it here and on flake's as well if you want

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T12:47:38Z

.github/workflows/ci.yml

+        id: changes
+        shell: bash
+        run: |
+          BASE_SHA="${{ github.event.pull_request.base.sha }}"


The BASE_SHA will be empty for push events since github.event.pull_request doesn't exist. This will cause the git diff command to fail. Use a conditional to handle both pull request and push events differently, or use github.event.before for push events.

Suggested change

BASE_SHA="${{ github.event.pull_request.base.sha }}"

if [ "${{ github.event_name }}" = "pull_request" ]; then

BASE_SHA="${{ github.event.pull_request.base.sha }}"

else

BASE_SHA="${{ github.event.before }}"

fi

@dorde-antic this comment looks like a valid issue. can you check ?

Yes, btw one thing about push events is that we didn't test them (since it would mean pushing to develop to try it) do you want that we keep push events as well? I think it make sense to have them since code can be changed that way too, the only thing is I'm not sure how to test it without spamming the commit history on develop

We can skip testing on push for now (you can monitor for a few pushes later) but my concern is that we should include path to get SHA for the push events as the copilot suggested.

.github/workflows/ci.yml

Copilot · 2025-09-19T12:47:39Z

.githooks/pre-commit

+set -eou pipefail
+
+NEED_POP=0
+if ! git diff --quiet || ! git diff --quiet -- .; then


The condition ! git diff --quiet -- . is redundant since ! git diff --quiet already checks the entire working directory. The second condition can be removed.

Suggested change

if ! git diff --quiet || ! git diff --quiet -- .; then

if ! git diff --quiet; then

.githooks/pre-commit

Initial commit

6b3a3ac

dorde-antic force-pushed the py-tidyandformatter branch from f3a4103 to 6b3a3ac Compare September 8, 2025 11:42

dorde-antic added 4 commits September 9, 2025 20:47

Change CI workflow to trigger on develop branch

01ea601

Merge branch 'develop' into py-tidyandformatter

41ec5d8

Consider only changes from PR

2bf3a8a

add test file

74f30bd

dorde-antic marked this pull request as ready for review September 9, 2025 19:18

dorde-antic requested a review from causten as a code owner September 9, 2025 19:18

dorde-antic requested review from Copilot, dhernandez0, mirza-halilcevic and umangyadav September 9, 2025 19:19

Copilot AI reviewed Sep 9, 2025

View reviewed changes

pip_requirements.txt Outdated Show resolved Hide resolved

mlir/utils/performance/dummyfile.py Outdated Show resolved Hide resolved

mlir/utils/performance/dummyfile.py Outdated Show resolved Hide resolved

dorde-antic and others added 3 commits September 9, 2025 21:32

Update mlir/utils/performance/dummyfile.py

a88072b

Co-authored-by: Copilot <[email protected]>

Update pip_requirements.txt

5fca811

Co-authored-by: Copilot <[email protected]>

Merge branch 'develop' into py-tidyandformatter

f8c0292

umangyadav reviewed Sep 10, 2025

View reviewed changes

dorde-antic added 4 commits September 11, 2025 14:57

Merge branch 'develop' into py-tidyandformatter

48ee267

Update dummyfile.py

b4c2a3e

Change YAPF style configuration to Google style

1e946d5

Merge branch 'develop' into py-tidyandformatter

f3d214f

dorde-antic added 3 commits September 13, 2025 17:16

Exclude llvm-project files only

3029281

TEMPORARY FILES - for testing

5a941e2

Move test files in dir that should be checked

ced1426

dorde-antic commented Sep 13, 2025

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

dorde-antic added 18 commits September 14, 2025 18:51

Update CI workflow to include external paths

496233b

test after applying google style with yapf -i

6fc769a

Add 'release/**' branch to CI workflow triggers

302f141

Change indent width from 2 to 4 spaces

aa6db02

Test files

8d4865e

test...

1ff1269

Merge branch 'develop' into py-tidyandformatter

e229364

...

246555d

add default_stages

a782700

Restage after yapf -i

e790568

Change pre-commit logic

4c93d44

test new githook

b22abf8

Delete mlir/randomTools.py

1fedd52

Delete mlir/checkForDuplicates.py

0e27043

Delete mlir/utils/performance/dummyfile.py

04b6548

Delete external/llvm-project/temp-test-file.py

5a2c1c1

revert perfrunner changesd

e164d29

Ignore E501 for flake

409f934

dorde-antic requested a review from umangyadav September 19, 2025 12:38

umangyadav reviewed Sep 19, 2025

View reviewed changes

umangyadav approved these changes Sep 19, 2025

View reviewed changes

umangyadav requested a review from Copilot September 19, 2025 12:46

Copilot AI reviewed Sep 19, 2025

View reviewed changes

umangyadav reviewed Sep 19, 2025

View reviewed changes

.githooks/pre-commit Show resolved Hide resolved

dorde-antic and others added 3 commits September 20, 2025 13:01

Merge branch 'develop' into py-tidyandformatter

cb0d1ad

Address copilot's comment for BASE_SHA

28ceb36

Merge branch 'develop' into py-tidyandformatter

d898254

umangyadav merged commit 2363cbb into develop Sep 22, 2025
8 of 22 checks passed

umangyadav deleted the py-tidyandformatter branch September 22, 2025 12:12

	if ! git diff --quiet \|\| ! git diff --quiet -- .; then
	if ! git diff --quiet; then

Python tidy and formatter #1978

Python tidy and formatter #1978

Uh oh!

Conversation

dorde-antic commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Test Plan

1. Github Actions CI Testing

2. Local Testing

3. Github Actions CI + pre-commit

Test Result

Submission Checklist

Uh oh!

codecov bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorde-antic commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhernandez0 commented Sep 10, 2025

Uh oh!

dorde-antic commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorde-antic commented Sep 13, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dorde-antic commented Sep 8, 2025 •

edited

Loading

codecov bot commented Sep 8, 2025 •

edited

Loading

dorde-antic commented Sep 9, 2025 •

edited

Loading