Update GitHub Actions for convention (`.pre-commit-config.yml`)

[namgyu-youn]

Some coding conventions are introduced at the following :

• Add pre-commit hook for linting and formatting #56 : .pre-commit-config.yml
• Add pre-commit hook for linting and formatting #56 : .github/workflows/lint.yml
• Deactivate ruff formatting check from workflow #65 : .github/workflows/format.yaml

But GitHub Actions (lint.yml & format.yml) is not progressed using .pre-commit-configs.yml. And there is To-do (make formatting with ruff consistent across internal and open-source code) in format.yml [Ln 21]. I assume this message is intended for future update using .pre-commit-configs.yml.

Therefore, I want to suggest GitHub Actions using pre-commit-config.yml for consistency. This update will introduce pre-commit.yml, and remove format.yml & lint.yml (filename could be changed).

Also, I want to ask if ruff version update (0.8.0 -> 0.11.2) could be processed. If the update is processed, then some files would be refactored for recent version.

[tsunghsienlee]

This is a little bit subtle because it was due to the inconsistencies between ruff used here and ruff used within Meta internal development system. Basically even though both using the same linter, ruff, with the same configurations we could find, the lint/formatting results are still differs; so we decide to stop those GitHub Actions to prevent unnecessary false-alarms from the internal commits.

[namgyu-youn]

This is a little bit subtle because it was due to the inconsistencies between ruff used here and ruff used within Meta internal development system. Basically even though both using the same linter, ruff, with the same configurations we could find, the lint/formatting results are still differs; so we decide to stop those GitHub Actions to prevent unnecessary false-alarms from the internal commits.

It seems that facebook-github-bot automatically runs an internal pre-commit before merging PR. I assume updating .pre-commit-config.yml based on Meta internal convention would be a fundamental solution, but there might be some reasons this is not progressed.

And because of the inconsistency, format.yml & lint.yml is not useful now. Therefore, it could even be stopped using markdown in the trigger.

This is not a fundamental solution, but I want to suggest GitHub Actions like PyTorch. And some markdowns would be added for explaining the subtle difference cleary.

[tsunghsienlee]

This is a little bit subtle because it was due to the inconsistencies between ruff used here and ruff used within Meta internal development system. Basically even though both using the same linter, ruff, with the same configurations we could find, the lint/formatting results are still differs; so we decide to stop those GitHub Actions to prevent unnecessary false-alarms from the internal commits.

It seems that facebook-github-bot automatically runs an internal pre-commit before merging PR. I assume updating .pre-commit-config.yml based on Meta internal convention would be a fundamental solution, but there might be some reasons this is not progressed.

And because of the inconsistency, format.yml & lint.yml is not useful now. Therefore, it could even be stopped using markdown in the trigger.

This is not a fundamental solution, but I want to suggest GitHub Actions like PyTorch. And some markdowns would be added for explaining the subtle difference cleary.

PyTorch format actually conflicts with Meta internal ruff setting based on few diffs I work on PyTorch through internal system; partially due to it is a Linux Foundation Project now so most of its developments are coming from external people. On the other hand, most of the developments of this optimizers projects will come from Meta internal instead.

[namgyu-youn]

PyTorch format actually conflicts with Meta internal ruff setting based on few diffs I work on PyTorch through internal system; partially due to it is a Linux Foundation Project now so most of its developments are coming from external people. On the other hand, most of the developments of this optimizers projects will come from Meta internal instead.

Thanks for the explanation; I think now I understand the issue better.

Then what about simply merging format.yml & lint.yml and minimizing jobs? Because both workflows make false alarms, they can be updated for minimized jobs.

Or, simply pausing them using markdown in trigger would be useful.

[tsunghsienlee]

There is no benefits of merging them because both the amount of computations is identical (if we ignore the thread bootstrap and tear down consumptions).

It will be ideal to make format.yaml formats users file without complaining, and leave those could not be formatted checks to others checks (if there is a obvious typo, others checks should complain).

[namgyu-youn]

There is no benefits of merging them because both the amount of computations is identical (if we ignore the thread bootstrap and tear down consumptions).

Let me rephrase my message. I think inconsistency exists on the contributor's side. Based on CONTRIBUTING.md, the convention must be .pre-commit-config.yml. Therefore, contributors should check pre-commit run --all-files before opening PR.

And there are GitHub Actions (format.yml & lint.yml) for maintainers, checking if contributors complied with convention. But GitHub Actions are not the same as .pre-commit-config.yml. For more explaination :

• pre-commit-config.yml checks ruff (linter & formater) and usort.
• format.yml checks usort
• lint.yml checks linter in ruff (no formatter)

For that reason, GitHub Actions doesn't check ruff - format case. This is the inconsistency I want to point out. And I assume To-Do in format.yml [Ln 21] is intended for this.

It will be ideal to make format.yml formats users file without complaining, and leave those could not be formatted checks to others checks (if there is a obvious typo, others checks should complain).

There is no typo because ruff -format is excluded in GitHub Actions. That is why no one has reported the issue.

I think GitHub Actions should check based on .pre-commit-config.yml because it is configured in CONTRIBUTING.md. Therefore, as mentioned before, I want to suggest merged GitHub Actions using pre-commit.yml.

[tsunghsienlee]

There is no benefits of merging them because both the amount of computations is identical (if we ignore the thread bootstrap and tear down consumptions).

Let me rephrase my message. I think inconsistency exists on the contributor's side. Based on CONTRIBUTING.md, the convention must be .pre-commit-config.yml. Therefore, contributors should check pre-commit run --all-files before opening PR.

And there are GitHub Actions (format.yml & lint.yml) for maintainers, checking if contributors complied with convention. But GitHub Actions are not the same as .pre-commit-config.yml. For more explaination :

• pre-commit-config.yml checks ruff (linter & formater) and usort.
• format.yml checks usort
• lint.yml checks linter in ruff (no formatter)

For that reason, GitHub Actions doesn't check ruff - format case. This is the inconsistency I want to point out. And I assume To-Do in format.yml [Ln 21] is intended for this.

Ideally we definitely want the consistency between GitHub Actions match with pre-commit-config.yml. However, we are not able to config ruff setting on GitHub to match with what Meta internal formatting due to the black box nature of how Meta configures its internal ruff setting. As a result, `ruff format --check has quite a few false-alarms every time when we port internal codes to the external. To me, if we could not make both parties consistent together, at least both are consistent with their own perspectives.

It will be ideal to make format.yml formats users file without complaining, and leave those could not be formatted checks to others checks (if there is a obvious typo, others checks should complain).

There is no typo because ruff -format is excluded in GitHub Actions. That is why no one has reported the issue.

I misunderstood what format.yaml does (thought it was doing the actual formatting file works) but instead it is checking the format. The typo checks I meant before is something very obvious typos like instead of writing for a, b in zip(a_list, b_list), a typo might be like for a, b in zpi(a_list, b_list) this kind of typos. Basically not runnable codes, and I believe things like this will be detected by many other checks.

I think GitHub Actions should check based on .pre-commit-config.yml because it is configured in CONTRIBUTING.md. Therefore, as mentioned before, I want to suggest merged GitHub Actions using pre-commit.yml.

Are you saying we could instruct some of our GitHub Actions (the one under .github/workflows) with .pre-commit-config.yaml? If that is the case, this prevent some functional-duplicate yamls if no false-alarms when considering that there are two different formatting systems, I am pretty sure this is welcome.