[Python] Migrate from yapf to black by sergiitk · Pull Request #33138 · grpc/grpc

sergiitk · 2023-05-16T02:51:04Z

Switched from yapf to black
Reconfigure isort for black
Resolve black/pylint idiosyncrasies

Note: I used --experimental-string-processing because black was producing "implicit string concatenation", similar to what described here: psf/black#1837. While currently this feature is experimental, it will be enabled by default: psf/black#2188. After running black with the new string processing so that the generated code merges these "hello" " world" strings concatenations, then I removed --experimental-string-processing for stability, and regenerated the code again.

To the reviewer: don't even try to open "Files Changed" tab 😄 It's better to review commit-by-commit, and ignore run black and isort.

sergiitk · 2023-05-16T07:12:45Z

Sorry, had to force-push to preserve the commit structure.

sergiitk · 2023-05-16T07:35:21Z

Yea, sorry, force pushes will have to happen in this PR.

sergiitk · 2023-05-16T07:38:41Z

In both cases I added more autogenerated files to the exclude list, re-ran black, and replaced the run black and isort commit. Otherwise, I'd have to manually find and restore each autogenerated file, which is prone to errors.

sergiitk · 2023-05-16T17:36:42Z

Per @ctiller's recommendation, we're postponing this merge until after the end of the fixit. Then I'll sync it with master, and redo the code generation.

XuanWang-Amos · 2023-05-16T17:00:32Z

pyproject.toml

+src_paths = [
+    "examples/python/data_transmission",
+    "examples/python/async_streaming",
+    "tools/run_tests/xds_k8s_test_driver",
+    "src/python/grpcio_tests",
+    "tools/run_tests",
+]


I guess this part it copied from tools/distrib/isort_code.sh, @gnossen do you know why only those paths are included?

For the examples, I think the answer is basically "Lidi wrote these examples." I'm surprised that the implementation directory is not included here, but if it never was to begin with, I'm fine with leaving this the way it is.

Yea, that's weird. I just translated all the inline arguments to isort into a nice and readable config as is. The only change to isort settings is to use black profile, for obvious reasons.

XuanWang-Amos · 2023-05-19T21:24:48Z

tools/run_tests/xds_k8s_test_driver/bin/black.sh

 else
-  readonly MODE="--in-place"
-  readonly VERBOSE="--verbose" # print out file names while processing
+  readonly MODE=""


What will this default mode do?

Black in the default mode (no args) will rewrite the code in place. yapf needed --in-place to do that, otherwise it'd just print the formatted file contents to the stdout.

gnossen

Huge thanks for this. We've been wanting this for literally years.

pyproject.toml

gnossen · 2023-05-26T17:41:51Z

pyproject.toml

+src_paths = [
+    "examples/python/data_transmission",
+    "examples/python/async_streaming",
+    "tools/run_tests/xds_k8s_test_driver",
+    "src/python/grpcio_tests",
+    "tools/run_tests",
+]


For the examples, I think the answer is basically "Lidi wrote these examples." I'm surprised that the implementation directory is not included here, but if it never was to begin with, I'm fine with leaving this the way it is.

sergiitk · 2023-06-09T08:17:07Z

Just a note: The last force push was regenerating the code after with target-version updated to py37...py311. It had no effect on the generated code (and that's OK).

sergiitk · 2023-06-09T08:19:36Z

PSM Interop (using reformatted framework and rebuilding the test images from this branch):

sergiitk · 2023-06-09T08:27:28Z

@XuanWang-Amos Note I made a small change to src/python/grpcio/grpc/_observability.py - after reformatting pylint was complaining on

src/python/grpcio/grpc/_observability.py:33:4: E0601: Using variable 'ObservabilityPlugin' before assignment (used-before-assignment)

I just used the typehint notation made for this case - putting the class declared later in the same file in quotes.

Reverts #33385 Temporary revert to unblock #33138

PR #33138 is the Large Scale Change in which python style is changed from yapf to black. Nearly all python files were reformatted. This PR configures git clients supporting `.git-blame-ignore-revs` feature to show the original author in `git blame`. Details: https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html GitHub uses this feature by default: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view Local git clients can enable this feature with `git config blame.ignoreRevsFile .git-blame-ignore-revs`.

Reverts grpc#33385 Temporary revert to unblock grpc#33138

- Switched from yapf to black - Reconfigure isort for black - Resolve black/pylint idiosyncrasies Note: I used `--experimental-string-processing` because black was producing "implicit string concatenation", similar to what described here: psf/black#1837. While currently this feature is experimental, it will be enabled by default: psf/black#2188. After running black with the new string processing so that the generated code merges these `"hello" " world"` strings concatenations, then I removed `--experimental-string-processing` for stability, and regenerated the code again. To the reviewer: don't even try to open "Files Changed" tab 😄 It's better to review commit-by-commit, and ignore `run black and isort`.

PR grpc#33138 is the Large Scale Change in which python style is changed from yapf to black. Nearly all python files were reformatted. This PR configures git clients supporting `.git-blame-ignore-revs` feature to show the original author in `git blame`. Details: https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html GitHub uses this feature by default: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view Local git clients can enable this feature with `git config blame.ignoreRevsFile .git-blame-ignore-revs`.

jtattermusch · 2023-07-18T09:42:50Z

@sergiitk It would be good if the PR description contained info on what is the right command to run now (instead of tools/distrib/yapf_format.sh)

sergiitk · 2023-07-18T19:15:24Z

@jtattermusch Sorry, this was a large PR, easy to miss stuff.

tools/distrib/yapf_code.sh was replaced by tools/distrib/black_code.sh. They are the scripts themselves are compatible API- (argument-) wise. See a7447dc.

- Switched from yapf to black - Reconfigure isort for black - Resolve black/pylint idiosyncrasies Note: I used `--experimental-string-processing` because black was producing "implicit string concatenation", similar to what described here: psf/black#1837. While currently this feature is experimental, it will be enabled by default: psf/black#2188. After running black with the new string processing so that the generated code merges these `"hello" " world"` strings concatenations, then I removed `--experimental-string-processing` for stability, and regenerated the code again. To the reviewer: don't even try to open "Files Changed" tab 😄 It's better to review commit-by-commit, and ignore `run black and isort`.

sergiitk added area/tools lang/Python disposition/DO NOT MERGE release notes: no Indicates if PR should not be in release notes and removed disposition/DO NOT MERGE labels May 16, 2023

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels May 16, 2023

sergiitk force-pushed the python-black branch 3 times, most recently from c13dcbb to e00ba8f Compare May 16, 2023 04:39

github-actions bot added lang/C# lang/c++ lang/core lang/ObjC lang/php labels May 16, 2023

sergiitk force-pushed the python-black branch from dd5a756 to a5db4dd Compare May 16, 2023 06:24

sergiitk marked this pull request as ready for review May 16, 2023 06:35

sergiitk requested review from XuanWang-Amos and gnossen May 16, 2023 06:35

sergiitk force-pushed the python-black branch from a5db4dd to 7d45c17 Compare May 16, 2023 07:12

sergiitk force-pushed the python-black branch from 7d45c17 to aa8f7ae Compare May 16, 2023 07:34

gnossen assigned gnossen and XuanWang-Amos May 17, 2023

XuanWang-Amos reviewed May 19, 2023

View reviewed changes

gnossen reviewed May 26, 2023

View reviewed changes

sergiitk added 8 commits June 9, 2023 01:08

Configure isort for black

c3ee759

Experimental string processing

1e1c125

Run black and isort

f8a9206

Unset experimental string processing

6a3b941

Re-run black and isort with experimental string processing disabled

6acbe17

Fix pylint/black integration quirks

f1ed694

Rename pyptoject.toml to black.toml to avoid build confusions

ca85f76

Also update xds-k8s black/isort helpers

75a1b83

sergiitk force-pushed the python-black branch from 1b68c72 to 75a1b83 Compare June 9, 2023 08:13

sergiitk requested review from XuanWang-Amos and gnossen June 9, 2023 08:24

gnossen approved these changes Jun 9, 2023

View reviewed changes

veblush mentioned this pull request Jun 9, 2023

Revert "[Test] Dedup vs2019 test in the Windows test" #33393

Merged

veblush added a commit that referenced this pull request Jun 9, 2023

Revert "[Test] Dedup vs2019 test in the Windows test" (#33393)

e9e5dc0

Reverts #33385 Temporary revert to unblock #33138

sergiitk merged commit de6ed9b into grpc:master Jun 9, 2023

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jun 9, 2023

sergiitk deleted the python-black branch June 9, 2023 22:41

sergiitk added a commit to sergiitk/grpc that referenced this pull request Jun 9, 2023

[Meta] Ignore python style LSC grpc#33138 in git blame

03ac337

sergiitk mentioned this pull request Jun 9, 2023

[git] Ignore python style LSC #33138 in git blame #33396

Merged

mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023

Revert "[Test] Dedup vs2019 test in the Windows test" (grpc#33393)

c6f1950

Reverts grpc#33385 Temporary revert to unblock grpc#33138

sergiitk mentioned this pull request Jul 18, 2023

[python] It should be an error to have unformatted code #33752

Merged

Conversation

sergiitk commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergiitk commented May 16, 2023

Uh oh!

sergiitk commented May 16, 2023

Uh oh!

sergiitk commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergiitk commented May 16, 2023

Uh oh!

XuanWang-Amos May 16, 2023

Choose a reason for hiding this comment

Uh oh!

gnossen May 26, 2023

Choose a reason for hiding this comment

Uh oh!

sergiitk Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

XuanWang-Amos May 19, 2023

Choose a reason for hiding this comment

Uh oh!

sergiitk Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

gnossen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnossen May 26, 2023

Choose a reason for hiding this comment

Uh oh!

sergiitk commented Jun 9, 2023

Uh oh!

sergiitk commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergiitk commented Jun 9, 2023

Uh oh!

jtattermusch commented Jul 18, 2023

Uh oh!

sergiitk commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sergiitk commented May 16, 2023 •

edited

Loading

sergiitk commented May 16, 2023 •

edited

Loading

sergiitk commented Jun 9, 2023 •

edited

Loading