Document how to use yapf #2160

alok · 2018-05-30T02:21:18Z

This is a cleaner version of #2158. @ericl Please take a look at this.

AmplabJenkins · 2018-05-30T02:52:41Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5703/
Test FAILed.

ericl · 2018-05-30T04:38:22Z

doc/source/development.rst

+5. **Autoformatting code**. We use ``yapf``
+   https://github.com/google/yapf for linting, and the config file is
+   located at ``.style.yapf``. We recommend adding ``.travis/yapf.sh``
+   to git's ``pre-push`` hook, so that you never have to worry about


I'm curious why this is so slow, we only have ~90k lines of Python (even including rllib / dataframes), so it seems weird that it takes almost a minute to format that. I don't think we can recommend this unless we get formatting time down to some thing more reasonable, e.g. 5-10 seconds.

Takes about 20 seconds on my MacBook

Even a minute isn't that bad for pre push since you push far less than you commit

It must depend on your hardware then, on my laptop it takes twice a long (4x i7 cores). Still, 20s is an eternity when pushing typically takes just a couple seconds.

Have we considered any faster formatters?

The only one that's faster is black, since it checks if the files on disk have been changed.

I would be perfectly happy to switch to it, since it offers no configuration at all, which makes debates about style moot, except I doubt the rest of the team would, given the lack of said configuration.

FWIW, I've always been doing git status and then passing all changed files into

yapf --style=.style.yapf -i [all changed files]

And then I run flake8 after that in order to catch remaining errors (like unused imports).

ericl · 2018-05-30T04:40:45Z

doc/source/development.rst

+   to git's ``pre-push`` hook, so that you never have to worry about
+   your code formatting ever again. Note that RLlib is not currently
+   autoformatted, though running ``yapf`` over the individual files you
+   edit with ``yapf -i -p -r <file1> <file2> <...>`` is a good idea.


How about this instead?

yapf -i -p `git diff --name-only upstream/master | grep .py`

Not sure it's actually a good idea to run yapf on the files you edit because that will introduce a lot of changes in places you didn't mean to change and will make it hard to see what your PR is doing.

Hm I see, perhaps the --exclude flags would work here? Though then the cmd line becomes a bit long so you probably want to check in the script.

It would be great to have a ./yapf script that formats all relevant files and also is fast enough to run all the time.

This problem is actually why I've been pushing so annoyingly hard for autoformat everywhere, all the time. If we bit the bullet, formatted all the python code and read the ugly PR, and then put a pre-commit hook (we could copy this script from go), no change in the codebase would ever introduce line noise, because it would always be formatted the same. Since we'd only ever format changed files, it'd be fast enough to be unnoticeable too. It could later open up the possibility of mechanical source transformation.

A pre-push hook is a halfway measure, but is still far better than what's currently done. It also saves having to write overly complex scripts for what's really a problem with standards.

30k lines of ray were autoformatted without any major issues. I think the rest of the python code could be done too. I'd be happy to review, since code formatting hardly requires deep knowledge of the actual code being run.

Any PRs against master could run yapf in their final commit and then only the diff would appear in the github PR review tool, so it wouldn't be a pain to review.

You can still add a pre-commit hook, just with --excludes for now, no?

FWIW I think the full linting will happen, it's just that we need to find some quiescent time for the sub-projects and then someone can do a quick 3am push to reformat everything.

ericl · 2018-05-30T04:41:02Z

.travis/yapf.sh

+
+# This flag lints individual files. --files *must* be the first command line
+# arg to use this option.
+if [[ "$1" == '--files' ]]; then


Why this change? --files isn't used below.

This is a poor man's argparse since I hate messing with bash's getopts. If --files is the first argument, then only the files passed are formatted.

The new default is for the script to only updated changed files to encourage using it as a pre-push hook. Travis still checks all since it's not that big an increase to runtime.

alok · 2018-05-30T08:05:02Z

@ericl Can you see if the latest commit sped it up at all? Try .travis/yapf.sh --all.

AmplabJenkins · 2018-05-30T09:15:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5713/
Test PASSed.

ericl · 2018-05-30T17:36:10Z

It's still 25 seconds. Why not combine the changed file detection and --all though? That makes it instantaneous for most PRs.

Btw you should add a symlink from ~/.travis/yapf.sh to ~/yapf.sh, since the .travis files are hidden for most developers.

alok · 2018-05-30T17:47:20Z

I guess it makes sense to format just the files that differ from `master`, since we almost always end up merging into there anyway. That's few enough files to be instant, so it does double duty as a pre-push and pre-commit hook.

Hidden directories may get glossed over otherwise.

They are symlinks to the same thing, but `scripts` is more dev-friendly, while `.travis` is really only for Travis CI.

Most devs will only need `format_changed`, and this is run by default. `format_changed` should be fast enough in most cases to work as a pre-commit hook.

alok · 2018-05-30T18:00:25Z

@ericl The latest script should do what you want, can you test it out?

1. Mention how yapf can be used a pre-commit hook 2. rm `bash`, script is executable

ericl · 2018-05-30T18:30:10Z

.travis/yapf.sh

+    git diff --name-only HEAD~1 HEAD | grep '\.py$' | xargs -P 5 \
+        yapf \
+        --style "$ROOT/.style.yapf" \
+        --in-place --recursive --parallel


missing --excludes

Oops, didn't push the actual latest version, that's changed. I'd really rather avoid adding an --excludes flag. If someone wants to lint specific files/dirs, they can run yapf.sh --files <files or dirs>.

ericl · 2018-05-30T18:33:06Z

.travis/yapf.sh

-lint_all() {
+format_changed() {
+    # Formats python files that changed in last commit
+    git diff --name-only HEAD~1 HEAD | grep '\.py$' | xargs -P 5 \


This isn't quite right. Usually you want to diff from the last master commit, not the one previous.

ericl · 2018-05-30T18:35:32Z

doc/source/development.rst

-   your code formatting ever again. Note that RLlib is not currently
-   autoformatted, though running ``yapf`` over the individual files you
-   edit with ``.travis/yapf.sh --files <file1> <file2> <...>`` is a good idea.
+5. **Autoformatting code**. We use ``yapf`` https://github.com/google/yapf for


5. **Autoformatting code**. We use ``yapf`` https://github.com/google/yapf for linting, and the config file is located at ``.style.yapf``. We recommend running ``.travis/yapf.sh`` prior to pushing to format changed files. Note that some projects such as dataframes and rllib are currently excluded.

Can you make the above workflow work?

It should with the latest change.

ericl

I pushed some suggested changes. Let me know if I'm missing anything.

alok · 2018-05-30T19:13:34Z

.travis/yapf.sh

+--exclude 'python/ray/core/src/ray/gcs' \
+--exclude 'python/ray/common/thirdparty'"
+
+UPSTREAM_MASTER=${RAY_UPSTREAM_BRANCH:-upstream/master}


This doesn't need an env variable, and upstream/master isn't always defined (my remotes are alok and origin). Setting this to master or origin/master is a better idea because master is meant to track the main branch.

Changed to origin/master. Note that you can override the target here by setting RAY_UPSTREAM_BRANCH.

Actually there's a better way to do this... WIP

Ok... now we always fetch the latest upstream, and grab the merge base, so the diff should always be correct.

alok · 2018-05-30T19:14:54Z

.travis/yapf.sh

+# Format files that differ from main branch
+format_changed() {
+    FILES=`git diff --name-only $UPSTREAM_MASTER -- '*.py'`
+    yapf $YAPF_FLAGS $YAPF_EXCLUDES -- $FILES


This function should not exclude anything, since if you want that sort of fine-grained control over what you format, you can look at the output from git status manually and just pass those files to format. This is meant to be a quick way to format all files that differ from the main branch.

If you want to do that, you can run with --files. Consider the common use case. Would a user be surprised if the auto-formatter formatted a file that is excluded?

Alright. However, the if statement guard is necessary since otherwise -- $FILES will expand to --, which often causes problems. It only works because yapf's -i flag errors out with empty filenames.

alok · 2018-05-30T19:18:53Z

I don't think adding variables like that is a good idea. format is just meant to format the files you pass it with the minimum number of flags set to do it in parallel and to follow the style file. It should not exclude anything passed to it, that should be up to the user. format_all should use those flags, but it's kind of pointless if it's the only function using the exclude.

ericl · 2018-05-30T19:20:48Z

An typical developer trying to use the yapf script isn't going to consider these things; they just want to run it and have it do the right thing, unconditionally. Having modes that don't obey excludes is just complicating the situation.

Note that --files will ignore the excludes since in that case the user is making their intent clear.

AmplabJenkins · 2018-05-30T19:23:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5722/
Test FAILed.

AmplabJenkins · 2018-05-30T19:48:35Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5720/
Test PASSed.

AmplabJenkins · 2018-05-30T20:36:22Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5725/
Test PASSed.

Playing fast and loose with whitespace in bash is a terrible idea.

alok · 2018-05-30T19:28:14Z

.travis/yapf.sh

+
+# Format files that differ from main branch
+format_changed() {
+    FILES=`git diff --name-only $UPSTREAM_MASTER -- '*.py'`


@ericl By the way, the backtick syntax is discouraged in all shells these days, $() is the correct form.

Sounds good

alok · 2018-05-30T19:31:01Z

.travis/yapf.sh

+# Format files that differ from main branch
+format_changed() {
+    FILES=`git diff --name-only $UPSTREAM_MASTER -- '*.py'`
+    yapf $YAPF_FLAGS $YAPF_EXCLUDES -- $FILES


Alright. However, the if statement guard is necessary since otherwise -- $FILES will expand to --, which often causes problems. It only works because yapf's -i flag errors out with empty filenames.

alok · 2018-05-30T21:35:52Z

.travis/yapf.sh

+--exclude 'python/ray/common/thirdparty'"
+
+# Add the upstream branch if it doesn't exist
+if [ ! -e $ROOT/.git/refs/remotes/upstream ]; then


@ericl FYI, you should use [[ in bash over [, since [[ can warn of syntax errors because it's part of the syntax rather than an external command.

Normally, the remote is called `origin`, but naming it explicit

robertnishihara · 2018-05-30T21:54:09Z

Speaking of shell script best practices, @mehrdadn showed me https://github.com/koalaman/shellcheck recently. It integrates with various editors which is pretty nice.

robertnishihara · 2018-05-30T21:55:10Z

scripts

@@ -0,0 +1 @@
+.travis


this was probably added by accident, right?

No, this is on purpose. scripts is (currently) a symlink for .travis that can later be pulled out (without much difficulty) into a proper repo for one-off dev/user scripts. All of those scripts are in .travis right now, so I just symlinked it.

alok · 2018-05-30T22:02:08Z

@robertnishihara Yeah I use shellcheck for every line of shell I write, plus shellharden.

AmplabJenkins · 2018-05-30T23:01:28Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5734/
Test PASSed.

ericl · 2018-05-31T06:33:51Z

.travis/yapf.sh

+format_changed() {
+    if ! git diff --diff-filter=ACM --quiet --exit-code 'upstream/master' 'HEAD' -- '*.py' &>dev/null; then
+        git diff --name-only 'upstream/master' 'HEAD' -- '*.py' | xargs -P 5 \
+            yapf "${YAPF_EXCLUDES[@]}" "${YAPF_FLAGS[@]}"


Why do you need the xargs here when the yapf flag includes --parallel?

ericl · 2018-05-31T06:34:30Z

.travis/yapf.sh

+# for autoformat yet.
+format_changed() {
+    if ! git diff --diff-filter=ACM --quiet --exit-code 'upstream/master' 'HEAD' -- '*.py' &>dev/null; then
+        git diff --name-only 'upstream/master' 'HEAD' -- '*.py' | xargs -P 5 \


Shouldn't this be diffing against the mergebase? Have you checked what happens if master gets ahead of the current branch?

ericl · 2018-05-31T06:34:54Z

.travis/yapf.sh

+# Format files that differ from main branch. Ignores dirs that are not slated
+# for autoformat yet.
+format_changed() {
+    if ! git diff --diff-filter=ACM --quiet --exit-code 'upstream/master' 'HEAD' -- '*.py' &>dev/null; then


Let's avoid the double diff here. Just pipe the output directly into yapf.

ericl · 2018-05-31T06:35:21Z

.travis/yapf.sh

+}
+
+# Formats *all* files that differ from main branch.
+format_all_changed() {


This is unused. Please remove.

ericl · 2018-05-31T06:35:53Z

.travis/yapf.sh

+elif [[ "$1" == '--all' ]]; then
+    format_all
+else
+    # Format only the files that changed in last commit.


changed from the upstream mergebase

Remove unused function and move around code to make clearer and adding lines give cleaner diffs.

AmplabJenkins · 2018-05-31T22:26:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5784/
Test PASSed.

Each arg has to be in its own set of quotes.

AmplabJenkins · 2018-06-01T00:11:41Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5787/
Test FAILed.

alok · 2018-06-03T04:39:17Z

bump @ericl

ericl · 2018-06-04T00:19:21Z

.travis/yapf.sh

+    # `diff-filter=ACM` is to ensure we only format files that exist on both
+    # branches.
+    if ! git diff --diff-filter=ACM --quiet --exit-code 'upstream/master' -- '*.py' &>dev/null; then
+        git diff --name-only --diff-filter=ACM 'upstream/master' -- '*.py' | yapf "${YAPF_EXCLUDES[@]}" "${YAPF_FLAGS[@]}"


When I ran this, yapf complained with

yapf: error: cannot use --in-place or --diff flags when reading from stdin

This seemed to be fixed if you do

yapf "${YAPF_EXCLUDES[@]}" "${YAPF_FLAGS[@]}" `git diff --name-only --diff-filter=ACM 'upstream/master' -- '*.py'`

instead

ericl · 2018-06-04T00:19:42Z

.travis/yapf.sh

+    #
+    # `diff-filter=ACM` is to ensure we only format files that exist on both
+    # branches.
+    if ! git diff --diff-filter=ACM --quiet --exit-code 'upstream/master' -- '*.py' &>dev/null; then


/dev/null, not dev/null. The latter doesn't run on my machine.

Oops, that's a typo.

ericl · 2018-06-04T00:21:04Z

.travis/yapf.sh

+    # could cause yapf to receive 0 positional arguments, making it hang
+    # waiting for STDIN.
+    #
+    # `diff-filter=ACM` is to ensure we only format files that exist on both


ACM is not sufficient to ensure the diff is from the correct mergebase. You can do a simple experiment to verify this.

check out a really old Ray branch that just changes 1-2 files

run yapf

observe it try to format dozens of files.

If you set the mergebase correctly, it will only format those 1-2 files. Please revert this to the mergebase code.

ericl · 2018-06-04T00:21:47Z

.travis/yapf.sh

+fi
+
+# Only fetch master since that's the branch we're diffing against.
+git fetch 'upstream' 'master'


nit: You don't need single quotes here around upstream or master.

ericl · 2018-06-04T00:21:58Z

.travis/yapf.sh

+
+# Add the upstream branch if it doesn't exist
+if ! [[ -e "$ROOT/.git/refs/remotes/upstream" ]]; then
+    git remote add 'upstream' 'https://github.com/ray-project/ray.git'


Nit: you don't need single quotes around these literals.

I put the single quotes to signify that those arguments are not part of the command git remote add, but that's a stylistic choice, so I'm happy to revert it.

TIL there's a clean syntax for doing that, but it's too clever to belong in a shell script. We use `mapfile -t` to ensure no problems down the line with weird filenames.

AmplabJenkins · 2018-06-04T02:12:17Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5843/
Test PASSed.

ericl · 2018-06-06T03:21:52Z

Just tried this and it works now. Thanks for fixing it!

ericl · 2018-06-06T03:22:43Z

doc/source/development.rst

-5. **Inspecting Redis shards by hand:** To inspect the primary Redis shard by
+5. **Autoformatting code**. We use ``yapf`` https://github.com/google/yapf for
+   linting, and the config file is located at ``.style.yapf``. We recommend
+   running ``.travis/yapf.sh`` prior to pushing to format changed files.


Ah, I guess we could also recommend scripts/yapf.sh, oh well.

I think we should just hold off until we can look at all the scripts that belong in .travis and that belong in scripts. I made the symlink more for convenience, I don't expect it to stick around.

alok added 3 commits May 29, 2018 19:21

Allow yapf to lint individual files

fe83410

Add tip for using yapf

0d740fa

Update doc

0216e41

ericl reviewed May 30, 2018

View reviewed changes

alok added 2 commits May 29, 2018 23:26

Update script to autoformat changed py files

47e5fb8

The new default is for the script to only updated changed files to encourage using it as a pre-push hook. Travis still checks all since it's not that big an increase to runtime.

Exclude formatting thirdparty/autogen py files

5ec66fe

alok added 3 commits May 30, 2018 10:50

Symlink .travis -> scripts

284e731

Hidden directories may get glossed over otherwise.

.travis -> scripts in docs

6f93d66

They are symlinks to the same thing, but `scripts` is more dev-friendly, while `.travis` is really only for Travis CI.

Document different yapf format functions

f5963ff

Most devs will only need `format_changed`, and this is run by default. `format_changed` should be fast enough in most cases to work as a pre-commit hook.

alok added 2 commits May 30, 2018 11:01

Speed up yapf by only formatting changed files

6e4c265

Update docs

20be587

1. Mention how yapf can be used a pre-commit hook 2. rm `bash`, script is executable

ericl reviewed May 30, 2018

View reviewed changes

ericl added 2 commits May 30, 2018 11:51

Update yapf.sh

ee89ff6

Update development.rst

d69089f

ericl reviewed May 30, 2018

View reviewed changes

alok commented May 30, 2018

View reviewed changes

Update yapf.sh

fdc8252

alok added 2 commits May 30, 2018 14:24

Use bash arrays for correct argument splitting

5a6534d

Playing fast and loose with whitespace in bash is a terrible idea.

Only format non-excluded by default

6dd5cce

alok commented May 30, 2018

View reviewed changes

alok force-pushed the doc-yapf branch from 1f54b25 to b2887bc Compare May 30, 2018 21:52

Check changes against master

b2887bc

Normally, the remote is called `origin`, but naming it explicit

robertnishihara reviewed May 30, 2018

View reviewed changes

ericl reviewed May 31, 2018

View reviewed changes

alok added 3 commits May 31, 2018 14:16

Adding missing directory to format_all

1bf9209

Cleanup YAPF code

81c55c2

Remove unused function and move around code to make clearer and adding lines give cleaner diffs.

Ensure correct files are autoformatted

d7398d5

Fix cmd line arg splitting

69546d9

Each arg has to be in its own set of quotes.

ericl requested changes Jun 4, 2018

View reviewed changes

Diff against mergebase

56af46c

TIL there's a clean syntax for doing that, but it's too clever to belong in a shell script. We use `mapfile -t` to ensure no problems down the line with weird filenames.

ericl approved these changes Jun 6, 2018

View reviewed changes

ericl merged commit 42a9233 into ray-project:master Jun 6, 2018

ericl reviewed Jun 6, 2018

View reviewed changes

alok deleted the doc-yapf branch June 11, 2018 10:55

Document how to use yapf #2160

Document how to use yapf #2160

Conversation

alok commented May 30, 2018

AmplabJenkins commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericl May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertnishihara May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alok commented May 30, 2018

AmplabJenkins commented May 30, 2018

ericl commented May 30, 2018

alok commented May 30, 2018 via email

alok commented May 30, 2018

ericl May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alok commented May 30, 2018

ericl commented May 30, 2018 • edited Loading

AmplabJenkins commented May 30, 2018

AmplabJenkins commented May 30, 2018

AmplabJenkins commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertnishihara commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alok commented May 30, 2018

AmplabJenkins commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented May 31, 2018

AmplabJenkins commented Jun 1, 2018

alok commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 4, 2018

ericl commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericl May 30, 2018 •

edited

Loading

robertnishihara May 30, 2018 •

edited

Loading

ericl May 30, 2018 •

edited

Loading

ericl commented May 30, 2018 •

edited

Loading