Update clang-format automation #25700

pietern · 2019-09-05T10:33:22Z

Stack from ghstack:

Bogus diff to trigger clang-format error #25701 Bogus diff to trigger clang-format error
Update clang-format automation #25700 Update clang-format automation

This change updates the tools/clang_format.py driver script to exit
with non-zero status on error, fix running with --check-all, use
some of the tricks that have been added to tools/clang_tidy.py over
time, and more.

It also:

Removes the jit paths from the whitelist (they are not formatted).
Updates the pre-commit hook to deal with the non-zero exit status.
Adds tools/run-clang-format-in-ci.sh.
Adds an Azure pipeline to run clang-format in CI.

This change updates the `tools/clang_format.py` driver script to exit with non-zero status on error, fix running with `--check-all`, use some of the tricks that have been added to `tools/clang_tidy.py` over time, and more. It also: * Removes the jit paths from the whitelist (they are not formatted). * Updates the pre-commit hook to deal with the non-zero exit status. * Adds `tools/run-clang-format-in-ci.sh`. * Adds an Azure pipeline to run clang-format in CI.

pietern · 2019-09-05T10:37:42Z

Seems to work, see: https://github.com/pytorch/pytorch/pull/25701/checks?check_run_id=212960270

This change updates the `tools/clang_format.py` driver script to exit with non-zero status on error, fix running with `--check-all`, use some of the tricks that have been added to `tools/clang_tidy.py` over time, and more. It also: * Removes the jit paths from the whitelist (they are not formatted). * Updates the pre-commit hook to deal with the non-zero exit status. * Adds `tools/run-clang-format-in-ci.sh`. * Adds an Azure pipeline to run clang-format in CI.

ezyang · 2019-09-05T14:54:58Z

Can we update the wiki with information about how to run clang-format locally? https://github.com/pytorch/pytorch/wiki

ezyang · 2019-09-05T14:58:45Z

tools/run-clang-format-in-ci.sh

+# Exits with non-zero status if clang-format generated changes.
+time python tools/clang_format.py \
+  --verbose                       \
+  --diff "$BASE_BRANCH"           \


Let's please run clang-format on all files in CI, not the diff only. Having it be diff-based in clang-tidy means that most of the time there is broken code in master and we don't know about it.

ezyang

CI formatter should run on all files. If that means that we have to blacklist some files to start, let's do that.

suo

The reason we never did this is because different version of clang-format produce different results. Notably the default version on fb devservers is at some random point (not a release). So unless we can figure out a way to get everyone using the same clang-format in their editor, this will cause a lot of noise.

pietern · 2019-09-05T17:58:33Z

@suo Thanks for the insight. I figured it was somehow progressive in nature.

I just checked and fb devservers have 8.0 installed, which is the same as run in the Azure pipeline.

suo · 2019-09-05T18:47:09Z

The version number that clang-format --version reports may be misleading, since fb does some weird stuff with their tooling versioning I think (this may have changed since I last looked at it though).

But even if devserver and azure is the same, we would need to get external contributors, devfairs, peoples' laptops, etc. to be on the same version. Generally people have been resistant to adding another hoop for OSS people to jump though to contribute to PyTorch (as I'm sure you know).

This is solvable-ish through tooling, for example Chrome stores canonical clang-format binaries for each platform and has a script to download+run the binary on the codebase. We could do the same.

ezyang · 2019-09-05T20:28:27Z

Can we just use Chrome's binaries? :)

suo · 2019-09-05T20:57:31Z

Haha, maybe—although they update once in a while atomically with running the new version on the whole codebase, so that would be annoying.

If we wanted to go that route, I think the work items would be:

Figure out what binary to use. It needs to be the one in fbcode, since otherwise people working fbcode-first will have a tough time. We need to figure out if there's some special rev fbcode tools are based on and compile that rev for platforms we care about.
Get binaries in S3
Write a script that downloads the binaries w/ caching and formats a whitelist (and gitignore the cached binary)
Figure out how to get everyone to run this script (required pre-push hook?), and as a bonus get the right binary in everyone's PATH "somehow"
Activate the CI that checks the whitelist
Expand the whitelist while trying not to run afoul of too many merge conflicts

ShahriarRezghi · 2019-09-06T18:12:21Z

@pietern A comment on the checking script. I have used a MIT licensed code in torchvision to check format (link) I think it is more suitable to use in this case. What do you think?

pietern · 2019-09-09T09:17:19Z

@ShahriarSS It looks like that script is also a clang-format runner. What makes it more suitable?

rohan-varma · 2019-09-17T18:32:15Z

tools/clang_format.py

-CLANG_FORMAT_WHITELIST = ["torch/csrc/jit/", "test/cpp/jit/"]
+CLANG_FORMAT_WHITELIST = [
+    "torch/csrc/distributed/",
+    "torch/lib/c10d/",


q: is there a way to check which directories are fully compliant (i.e. maybe by running clang-format recursively on them)? If there is, should we do that and add them here?

rohan-varma · 2019-09-17T18:34:43Z

tools/clang_format.py

            name_to_diff[filename] = diff

+    # If no files produced a clang-format diff, exit early.
+    if len(name_to_diff) == 0:


Since this appears to occur when the code is formatted correctly, should we add:
if VERBOSE: print('No formatting changes were found')

to distinguish this from the case where no files were detected? Might help users distinguish between errors caused by passing in files incorrectly versus not having any formatting issues.

facebook-github-bot · 2020-10-30T17:23:47Z

Hi @pietern!

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

pytorchbot · 2022-04-12T02:34:24Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
Stale pull requests will automatically be closed 30 days after being marked Stale

github-actions · 2022-06-11T03:46:44Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

pytorchbot added module: infra Relates to CI infrastructure module: lint Issues related to our Python/C++ lint rules (run by Travis) labels Sep 5, 2019

pietern mentioned this pull request Sep 5, 2019

Bogus diff to trigger clang-format error #25701

Closed

pietern requested review from ezyang, suo and zdevito September 5, 2019 10:40

ezyang reviewed Sep 5, 2019

View reviewed changes

ezyang requested changes Sep 5, 2019

View reviewed changes

suo requested changes Sep 5, 2019

View reviewed changes

zdevito removed their request for review September 10, 2019 20:05

rohan-varma reviewed Sep 17, 2019

View reviewed changes

pietern mentioned this pull request Sep 20, 2019

[distributed] add known worker ids to distributed autograd context #26324

Closed

pietern mentioned this pull request Oct 24, 2019

[distributed] cleanup dist autograd context on other nodes when it is released on one node #27951

Closed

pietern mentioned this pull request Nov 5, 2019

DDP should not set grad for globally unused params #28883

Closed

pytorchbot added the open source label Feb 19, 2020

yf225 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 19, 2020

skyline75489 mentioned this pull request Jan 25, 2021

Cleanup git branches #41232

Open

pytorchbot added Stale and removed Stale labels Apr 12, 2022

github-actions bot added the Stale label Jun 11, 2022

github-actions bot closed this Jul 11, 2022

facebook-github-bot deleted the gh/pietern/10/head branch August 10, 2022 14:18

Update clang-format automation #25700

Update clang-format automation #25700

Uh oh!

Conversation

pietern commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pietern commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Sep 5, 2019

Uh oh!

ezyang Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

pietern commented Sep 5, 2019

Uh oh!

suo commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Sep 5, 2019

Uh oh!

suo commented Sep 5, 2019

Uh oh!

ShahriarRezghi commented Sep 6, 2019

Uh oh!

pietern commented Sep 9, 2019

Uh oh!

rohan-varma Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

rohan-varma Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 30, 2020

Uh oh!

pytorchbot commented Apr 12, 2022

Uh oh!

github-actions bot commented Jun 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pietern commented Sep 5, 2019 •

edited

Loading

pietern commented Sep 5, 2019 •

edited

Loading

suo commented Sep 5, 2019 •

edited

Loading