Skip to content

Conversation

@cclauss
Copy link
Contributor

@cclauss cclauss commented Jan 22, 2025

  1. print() is a function in Python 3.
  2. New-style exceptions are required in Python 3.

@github-actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Jan 22, 2025

@llvm/pr-subscribers-github-workflow

Author: Christian Clauss (cclauss)

Changes

Find all Python SyntaxErrors.

Commits:

  1. Find SyntaxErrors.
  2. Fix SyntaxErrors.

Test results: https://github.com/cclauss/arm-toolchain/actions


Full diff: https://github.com/llvm/llvm-project/pull/123940.diff

1 Files Affected:

  • (added) .github/workflows/lint.yml (+15)
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
new file mode 100644
index 00000000000000..9364d6ea369ea0
--- /dev/null
+++ b/.github/workflows/lint.yml
@@ -0,0 +1,15 @@
+name: lint
+
+on: [push, pull_request]
+
+permissions:
+  contents: read
+
+jobs:
+  lint_python:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: astral-sh/ruff-action@v3
+        with:  # Ignore all ruff rules except Python Syntax Errors
+          args: "check --ignore=ALL"

@llvmbot llvmbot added the openmp:libomp OpenMP host runtime label Jan 22, 2025
Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

I'm not really sure this is something that we want. The vast majority of python files will be run (and thus checked for syntax) during pre and post commit testing, so this action would be redundant for those. Standalone scripts I'm not sure benefit much from being automatically checked.

The couple scripts that you have fixed here seem old enough/unused enough/untested enough that we might just want to remove them, although that would need to go through review with the maintainers of those projects.

Additionally, anything at the scale of the LLVM monorepo is hard to get right. Actions like this typically need to run on a diff rather than the entire repo due to time constraints and to prevent noise from other parts of the repo being broken. There are also often a lot of subtleties that are hard to get right. It took us a lot of effort to even just get our code formatting action nailed down to really high reliability due to several obscure issues.

@cclauss
Copy link
Contributor Author

cclauss commented Jan 22, 2025

anything at the scale of the LLVM monorepo is hard to get right

ruff lints all files in the codebase in 1 second with no filters and no cache.

In a compiled language these errors would not exist in the repo but in an interpreted language like Python, building an AST is required to find these errors. This PR chooses to do this with ruff which is written in Rust for speed.

Cloning the repo on GitHub's high performance network takes 1m 20 so it is surprising that you do not use https://pre-commit.com for the linting and formatting jobs because it implements the diff-only kind of processing that you mention as vital.

I would be delighted to delete any unused files.

@boomanaiden154
Copy link
Contributor

ruff lints all files in the codebase in 1 second with no filters and no cache.

That's quite impressive, and a little bit surprising.

In a compiled language these errors would not exist in the repo but in an interpreted language like Python, building an AST is required to find these errors. This PR chooses to do this with ruff which is written in Rust for speed.

The errors only get caught if the code gets compiled. Syntax errors in Python will get caught if the code runs. The vast majority of the Python in LLVM gets run by our premerge checks, and even more gets run post-submit. I don't think we're really lacking any significant coverage in terms of python syntax errors.

Cloning the repo on GitHub's high performance network takes 1m 20 so it is surprising that you do not use https://pre-commit.com/ for the linting and formatting jobs because it implements the diff-only kind of processing that you mention as vital.

I fail to see how precommit hooks help here. Our infrastructure is already diff-driven for a variety of reasons and set up through GHA for integration with Github.

I would be delighted to delete any unused files.

You need to open up separate patches per-subproject for that. It would be good to do anyways for fixing the syntax errors. Although it's quite difficult to figure out what is used and unused with a project like LLVM.

@cclauss
Copy link
Contributor Author

cclauss commented Jan 23, 2025

Syntax errors in Python will get caught if the code runs.

If that line of code runs... In summarizeStats.py line 40 will only crash on the end user if line 39 evaluates to True and line 146 will only crash if line 145 evaluates to True. This means that this file can be run repeatedly without detecting these syntax errors but then with a certain input or when a certain function is called they crash in production. These runtime crashes are quite different from and more dangerous than compile-time crashes.

I don't think we're really lacking any significant coverage in terms of python syntax errors.

If we can find 4 files in one second then coverage is incomplete and we should not sweep runtime crashes under the rug.

I would encourage you to use pre-commit in a smaller project to see the just-in-time value that it delivers.

Copy link
Member

@junlarsen junlarsen left a comment

Choose a reason for hiding this comment

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

If that line of code runs... In summarizeStats.py line 40 will only crash on the end user if line 39 evaluates to True and line 146 will only crash if line 145 evaluates to True. This means that this file can be run repeatedly without detecting these syntax errors but then with a certain input or when a certain function is called they crash in production. These runtime crashes are quite different from and more dangerous than compile-time crashes.

I think the fact that it hasn't been changed yet is a fair indication of how much use the script gets, but in any case I don't see any harm in updating it.

If we can find 4 files in one second then coverage is incomplete and we should not sweep runtime crashes under the rug.

Same as above, I don't think they get enough use to warrant a new linter, but I don't see a problem in updating them.

I would encourage you to use pre-commit in a smaller project to see the just-in-time value that it delivers.

We have a git plugin for clang-format and encourage developers to configure hook it up as a pre-push hook themselves https://llvm.org/docs/Contributing.html#git-pre-push-hook

From my personal experience pre-commit hooks can do just as much harm as good if they're not done right, and I just don't think there is enough Python code in the monorepo that sees enough use by non-automation to warrant a pre-commit hook or new infrastructure.

There's also thousands of developers working on LLVM, so enforcing any commit hooks should be done with great consideration

@cclauss
Copy link
Contributor Author

cclauss commented Jan 23, 2025

I have removed the GitHub Action but have left the fixes to the four Python files.

@cclauss cclauss changed the title GitHub Actions: Lint Python code for just for SyntaxErrors Fix four Python that contain SyntaxErrors Jan 23, 2025
@cclauss cclauss changed the title Fix four Python that contain SyntaxErrors Fix four Python files that contain SyntaxErrors Jan 23, 2025
Copy link

@jpeyton52 jpeyton52 left a comment

Choose a reason for hiding this comment

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

OpenMP changes to summarizeStats.py look good to me.

@cclauss cclauss changed the title Fix four Python files that contain SyntaxErrors OpenMP: Fix Python SyntaxErrors Jan 25, 2025
@cclauss
Copy link
Contributor Author

cclauss commented Jan 25, 2025

You need to open up separate patches per-subproject

Done.

@cclauss cclauss changed the title OpenMP: Fix Python SyntaxErrors OpenMP: Fix Python 3 SyntaxErrors Jan 26, 2025
Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

Can you update the PR description to remove irrelevant information rather than just crossing it out?

After that, I should be able to land this.

@github-actions
Copy link

github-actions bot commented Jan 27, 2025

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

Copy link
Contributor Author

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Darker prefers double quotes and spaces.

@boomanaiden154 boomanaiden154 merged commit 89c5576 into llvm:main Jan 27, 2025
6 checks passed
@cclauss cclauss deleted the patch-1 branch January 27, 2025 22:46
@github-actions
Copy link

@cclauss Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants