Skip to content

Conversation

namgyu-youn
Copy link
Contributor

@namgyu-youn namgyu-youn commented Apr 14, 2025

Inspired by pre-commit#2240 and chromium-bidi#1834, double-run for pre-commit could be useful for resolving pre-commit conflicts.

Also, SVG in README.md is updated beacuse it wasn't updated in (#133)

Although pre-comit is specified in CONTRIBUTING.md, some pre-commit worked indirectly, using makefile.
- GitHub Actions : Build integrated CI for pre-commit
- Makefile : Remove linting & formatting option
- CONTRIBUTING.md : Update guide for convention of pre-commit
)

There are two pre-commit and makes conflication like the following:
- External (GitHub) : .pre-commit-config.yaml
- Internal (Meta Internal System) : black-box (not public)

Because the order is "External -> Internal" and internal rule slightly differs from external rule, back-and-forth issue occurs in some files. Therefore, ignoring pre-commit could be useful.
SVG in README was not updated in facebookresearch#133
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 14, 2025
@tsunghsienlee
Copy link
Contributor

Is it possible to fix ruff-format to let it format but without complaining first? This was mentioned in #121 (comment) because I foresaw this conflicts between internal and external formatting. Ideally I want pre-commit does to format files but GitHub just acknowledge the internal setting is the dominant ones.

@namgyu-youn
Copy link
Contributor Author

Is it possible to fix ruff-format to let it format but without complaining first? This was mentioned in #121 (comment) because I foresaw this conflicts between internal and external formatting. Ideally I want pre-commit does to format files but GitHub just acknowledge the internal setting is the dominant ones.

Now I see the solution after reading some docs (ruff#15746, ruff#16009, ruff - docs). The point is, GitHub Actions (pre-commit.yaml) returns non-zero (1) value when safe reformatting is detected. Therefore, we can try "return zero(0) when safe reformatting is detected." (when there is no change because full codebase pass pre-commit run --all-files, zero is returned)

Lcukily, there are some parameters related to this issue:

--exit-zero
    Exit with status code "0", even upon detecting lint violations
--exit-non-zero-on-format
    Exit with a non-zero status code if any files were modified via
    format, even if all files were formatted successfully

And --exit-zero seems to be useful because it returns zero although safe reformat is detected.

@tsunghsienlee
Copy link
Contributor

Is it possible to fix ruff-format to let it format but without complaining first? This was mentioned in #121 (comment) because I foresaw this conflicts between internal and external formatting. Ideally I want pre-commit does to format files but GitHub just acknowledge the internal setting is the dominant ones.

Now I see the solution after reading some docs (ruff#15746, ruff#16009, ruff - docs). The point is, GitHub Actions (pre-commit.yaml) returns non-zero (1) value when safe reformatting is detected. Therefore, we can try "return zero(0) when safe reformatting is detected." (when there is no change because full codebase pass pre-commit run --all-files, zero is returned)

Lcukily, there are some parameters related to this issue:

--exit-zero
    Exit with status code "0", even upon detecting lint violations
--exit-non-zero-on-format
    Exit with a non-zero status code if any files were modified via
    format, even if all files were formatted successfully

And --exit-zero seems to be useful because it returns zero although safe reformat is detected.

Great, this is exactly I want. Really appreciate that you dig into this and find this solution to achieve a good balance for both internal and external developers!

@namgyu-youn
Copy link
Contributor Author

namgyu-youn commented Apr 14, 2025

Is it possible to fix ruff-format to let it format but without complaining first? This was mentioned in #121 (comment) because I foresaw this conflicts between internal and external formatting. Ideally I want pre-commit does to format files but GitHub just acknowledge the internal setting is the dominant ones.

Now I see the solution after reading some docs (ruff#15746, ruff#16009, ruff - docs). The point is, GitHub Actions (pre-commit.yaml) returns non-zero (1) value when safe reformatting is detected. Therefore, we can try "return zero(0) when safe reformatting is detected." (when there is no change because full codebase pass pre-commit run --all-files, zero is returned)
Lcukily, there are some parameters related to this issue:

--exit-zero
    Exit with status code "0", even upon detecting lint violations
--exit-non-zero-on-format
    Exit with a non-zero status code if any files were modified via
    format, even if all files were formatted successfully

And --exit-zero seems to be useful because it returns zero although safe reformat is detected.

Great, this is exactly I want. Really appreciate that you dig into this and find this solution to achieve a good balance for both internal and external developers!

Sorry I just found --exit-zero only works in check, and --exit-non-zero-on-format only works in format. It can't be resolved using --exit-zero; I have to read more docs...

@namgyu-youn namgyu-youn marked this pull request as draft April 14, 2025 18:51
This is intended for 'If something were auto-fixed, but otherwise everything is ok, still exit 0'
@namgyu-youn namgyu-youn reopened this Apr 15, 2025
namgyu-youn and others added 7 commits April 15, 2025 14:15
Summary:
There are the following CI issues for pre-commit because of conflicts between `pre-commit` and `make`:

1. Althought pre-commit (`.pre-commit-config.yaml`) exist, it isn't used in GitHub Actions (`format.yaml & lint.yaml`) (facebookresearch#121)
2. `Makefile` indirectly import `ruff & usort`, and version is not specified. Therefore, slightly difference caused by not-same version could happen in future. ([comment](facebookresearch#133 (comment)))
3. In `CONTRIBUTING.md`, `pre-commit install` is configured for contributors. But instead of `pre-commit run --all-files`, indirectly pre-commit (`make format & make lint`) are suggested. ([comment](facebookresearch#133 (comment)))

Therefore, CI focused on `pre-commit` could be more brief and also compensate for consistency within the GitHub repository.

- Close : facebookresearch#121

Pull Request resolved: facebookresearch#133

Reviewed By: chuanhaozhuge

Differential Revision: D72872945

Pulled By: tsunghsienlee

fbshipit-source-id: 0f2a48aa33209633bc5197a8b253f470ffffb371
Summary:
Pull Request resolved: facebookresearch#141

Using a local test-only invalid config class to replace mock usages is more resilent to application-side code changes.

Reviewed By: minddrummer

Differential Revision: D72743397

fbshipit-source-id: 2de2af44827dc307ecb0d4a908c1832d2e5fd5b6
Summary: This diff refactors the `QREigendecompositionConfig` test in the `matrix_functions_test.py` file. The code changes include refactoring the initialization strategies to functions dictionary and using a for loop to iterate over the dictionary.

Reviewed By: minddrummer

Differential Revision: D72768312

fbshipit-source-id: 8fff67ce7e74068970737a47e84a35157c5dfa24
Summary:
Pull Request resolved: facebookresearch#139

This is the equivalent of popular [`pytest.mark.parametrize`](https://docs.pytest.org/en/stable/example/parametrize.html) in PyTorch. This is the first try on using this in this project.

Reviewed By: minddrummer

Differential Revision: D72897224

fbshipit-source-id: 015ca821298358484d5fb039715a172d490b29ec
This is intended for 'If something were auto-fixed, but otherwise everything is ok, still exit 0'
@namgyu-youn namgyu-youn marked this pull request as ready for review April 15, 2025 05:23
@namgyu-youn
Copy link
Contributor Author

namgyu-youn commented Apr 15, 2025

Is it possible to fix ruff-format to let it format but without complaining first? This was mentioned in #121 (comment) because I foresaw this conflicts between internal and external formatting. Ideally I want pre-commit does to format files but GitHub just acknowledge the internal setting is the dominant ones.

Maybe we thought this issue too complex. Because both (internal & external) use ruff, just double-running pre-commit in GitHub Actions could be useful in this case.

There is the same issue in pre-commit#2240, and it is resolved in chromium-bidi#1834. It was addressed for the following desired change:

  if everything is green, exit 0
  if things were autofixed, but otherwise everything is ok, still exit 0
  if something is broken and/or not autofix-able, exit nonzero

--exit-zero could be useful for some cases, but I am fine to go with double-run.

@namgyu-youn namgyu-youn changed the title Update SVG for pre-commit Fix conflict (internal, external) for pre-commit and update SVG Apr 15, 2025
@tsunghsienlee
Copy link
Contributor

Is it possible to fix ruff-format to let it format but without complaining first? This was mentioned in #121 (comment) because I foresaw this conflicts between internal and external formatting. Ideally I want pre-commit does to format files but GitHub just acknowledge the internal setting is the dominant ones.

Maybe we thought this issue too complex. Because both (internal & external) use ruff, just double-running pre-commit in GitHub Actions could be useful in this case.

There is the same issue in pre-commit#2240, and it is resolved in chromium-bidi#1834. It was addressed for the following desired change:

  if everything is green, exit 0
  if things were autofixed, but otherwise everything is ok, still exit 0
  if something is broken and/or not autofix-able, exit nonzero

--exit-zero could be useful for some cases, but I am fine to go with double-run.

Got it, double-run definitely is a smart idea because each run of ruff-format is extremely fast and the formatting operation is idempotent.

@tsunghsienlee tsunghsienlee self-requested a review April 15, 2025 15:56
@facebook-github-bot
Copy link
Contributor

@tsunghsienlee has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@tsunghsienlee merged this pull request in 5034dc4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants