[DO NOT MERGE] Rewrite YAPF scripts #1971

alok · 2018-05-01T03:04:10Z

What do these changes do?

Fixes YAPF to use existing .style.yapf config file
Runs in parallel
Easier to read yapf.sh (IMO)

Related issue number

No.

Any options not set in the config file defer to pep8.

This runs faster thanks to the `--parallel` flag and YAPF's builtin features. It's also a little clearer (IMO), and was run through `shellcheck` to harden up against a few of bash's many gotchas. It also actually uses the .style.yapf in the project repo, which was previously ignored.

pcmoritz · 2018-05-02T02:27:34Z

@ericl @richardliaw Are these linting changes ok with you? They look good to me!

alok · 2018-05-02T02:30:43Z

Personally, I'd tweak the .style.yapf file a bit. I set mine up to imitate Elm's style guide in the areas where Python has no clear rules, and I think it's easier to read. I just used the pep8 style in this PR because that was what was specified before. I think it's harder to read this way though since it has odd indentation at times.

AmplabJenkins · 2018-05-02T02:56:07Z

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

alok · 2018-05-04T09:02:35Z

Regardless of the particular style used, we consider the updated script since it shaves about 30s off Travis builds.

cd to parent of script, then use git to find its own root.

AmplabJenkins · 2018-05-08T06:46:24Z

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

format with yapf --style pep8 to satisfy travis

AmplabJenkins · 2018-05-09T10:11:11Z

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

1. rm useless cd's 2. Unquote array since it turns out checking the truthiness of an array is a real pain in bash and is best done in Python or something. 3. rm useless `exit 0` since scripts exit with the status of the last command anyway.

AmplabJenkins · 2018-05-09T23:35:54Z

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

richardliaw

Some comments

richardliaw · 2018-05-10T04:02:59Z

python/ray/monitor.py

@@ -507,8 +506,8 @@ def run(self):
        log.debug("{} dead local schedulers, {} plasma managers total, {} "
                  "dead plasma managers".format(
                      len(self.dead_local_schedulers),
-                      (len(self.live_plasma_managers) +
-                       len(self.dead_plasma_managers)),
+                      (len(self.live_plasma_managers) + len(


this one is also a little odd..

@alok can we revert this?

richardliaw · 2018-05-10T04:03:42Z

python/ray/experimental/state.py

@@ -750,8 +750,8 @@ def micros_rel(ts):
                        "name":
                        "SubmitTask",
                        "args": {},
-                        "id": (parent_info["worker_id"] +
-                               str(micros(min(parent_times))))
+                        "id": (parent_info["worker_id"] + str(


not a big fan of these

richardliaw · 2018-05-10T04:03:57Z

python/ray/experimental/state.py

@@ -825,8 +825,8 @@ def micros_rel(ts):
                        "name":
                        "SubmitTask",
                        "args": {},
-                        "id": (parent_info["worker_id"] +
-                               str(micros(min(parent_times))))
+                        "id": (parent_info["worker_id"] + str(


not a big fan of these

@alok can we revert this?

Added some trailing commas to make certain blocks neater. This style uses more vertical space but is (IMO) significantly more readable for a lot of code since you can just sort of scan each block independently since they're clearly delineated from each other. Feel free to revert to the last commit

alok · 2018-05-10T06:50:03Z

@richardliaw See the latest commit where I apply the style specified in the config file. I went in and manually tweaked some code by adding trailing commas so the formatter would do the right thing. I think it fixed all your comments and hopefully looks fine for the rest. If not, feel free to revert it.

AmplabJenkins · 2018-05-10T07:11:47Z

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

richardliaw · 2018-05-11T00:51:37Z

OK I think there's too much vertical space as of this most recent commit; would you be willing to revert?

This reverts commit 4579266.

alok · 2018-05-11T01:08:19Z

Done.

AmplabJenkins · 2018-05-11T02:11:34Z

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

richardliaw

two requests to change; otherwise looks good overall.

richardliaw · 2018-05-15T19:42:27Z

python/ray/experimental/state.py

@@ -825,8 +825,8 @@ def micros_rel(ts):
                        "name":
                        "SubmitTask",
                        "args": {},
-                        "id": (parent_info["worker_id"] +
-                               str(micros(min(parent_times))))
+                        "id": (parent_info["worker_id"] + str(


@alok can we revert this?

richardliaw · 2018-05-15T19:42:47Z

python/ray/monitor.py

@@ -507,8 +506,8 @@ def run(self):
        log.debug("{} dead local schedulers, {} plasma managers total, {} "
                  "dead plasma managers".format(
                      len(self.dead_local_schedulers),
-                      (len(self.live_plasma_managers) +
-                       len(self.dead_plasma_managers)),
+                      (len(self.live_plasma_managers) + len(


@alok can we revert this?

alok · 2018-05-16T17:25:50Z

@richardliaw I'd rather tweak the formatting config until it ends up looking pretty to avoid ever having to worry about manual formatting. I want it to be reliable enough to become a git pre-push hook so no one ever has to comment about formatting again.

It's better to have the occasional long line than to have very odd looking code. The codebase has long lines anyway, so it's not like they're totally verboten.

This should hopefully prevent some of the odder formatting.

This style looks fine to me and doesn't generally have a bunch of whitespace. A few trailing commas to split lines correctly and it should be satisfactory to everyone.

alok · 2018-05-16T18:16:59Z

@richardliaw can you try out this style? i think it fixes (most) of your issues with the current one.

AmplabJenkins · 2018-05-16T18:47:01Z

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

These should really be separate anyway.

AmplabJenkins · 2018-05-16T21:10:30Z

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

richardliaw · 2018-05-17T06:06:18Z

Hey @alok how did this become a 4k line PR?

alok · 2018-05-17T06:10:00Z

I made the mistake of actually committing the linting changes

alok · 2018-05-17T06:11:04Z

Check out https://github.com/alok/ray/tree/yapf-take-2 for a clean version

richardliaw · 2018-05-17T07:52:36Z

OK great take-2 looks a lot better. Consider making a commit that reverts changes from
6d2a24b onwards and then rebasing take-2 onto this.

alok · 2018-05-17T18:11:14Z

@richardliaw I ended up making a new branch from master and applying the necessary changes there. See #2083.

alok and others added 3 commits April 30, 2018 20:02

Base YAPF on pep8

3cf4f70

Any options not set in the config file defer to pep8.

commit linting fixes

323be6f

Find project root with git rev-parse

b1153d3

cd to parent of script, then use git to find its own root.

alok added 2 commits May 9, 2018 09:05

fmt

f3f8f47

format with yapf --style pep8 to satisfy travis

Use pep8 style by default

0bd4745

alok closed this May 9, 2018

alok reopened this May 9, 2018

Improve script

b9ff97b

1. rm useless cd's 2. Unquote array since it turns out checking the truthiness of an array is a real pain in bash and is best done in Python or something. 3. rm useless `exit 0` since scripts exit with the status of the last command anyway.

richardliaw reviewed May 10, 2018

View reviewed changes

Revert "Try new formatting style"

3105a88

This reverts commit 4579266.

richardliaw requested changes May 15, 2018

View reviewed changes

richardliaw self-assigned this May 15, 2018

alok mentioned this pull request May 16, 2018

Use flake8-comprehensions #1976

Merged

alok added 2 commits May 16, 2018 17:51

Reduce penalty for going over column limit

6d2a24b

It's better to have the occasional long line than to have very odd looking code. The codebase has long lines anyway, so it's not like they're totally verboten.

Avoid splitting on open parens

d2a0ba7

This should hopefully prevent some of the odder formatting.

alok added 3 commits May 16, 2018 17:53

Make shell script more robust to cwd

45501cf

Lint pandas on ray and RLlib

42ec70a

Try chromium style for yapf

30f6478

This style looks fine to me and doesn't generally have a bunch of whitespace. A few trailing commas to split lines correctly and it should be satisfactory to everyone.

This was referenced May 16, 2018

[DO NOT MERGE] preview of chromium formatting #2073

Closed

[DO NOT MERGE] preview of pep8 formatting #2074

Closed

Revert all formatting changes

a1a5633

These should really be separate anyway.

alok mentioned this pull request May 17, 2018

YAPF, take 2 #2083

Closed

alok changed the title ~~Rewrite YAPF scripts~~ [DO NOT MERGE] Rewrite YAPF scripts May 17, 2018

alok mentioned this pull request May 19, 2018

YAPF, take 3 #2098

Merged

alok closed this May 19, 2018

alok deleted the yapf branch May 21, 2018 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] Rewrite YAPF scripts #1971

[DO NOT MERGE] Rewrite YAPF scripts #1971

alok commented May 1, 2018

pcmoritz commented May 2, 2018

alok commented May 2, 2018

AmplabJenkins commented May 2, 2018

alok commented May 4, 2018 •

edited

Loading

AmplabJenkins commented May 8, 2018

AmplabJenkins commented May 9, 2018

AmplabJenkins commented May 9, 2018

richardliaw left a comment

richardliaw May 10, 2018

richardliaw May 15, 2018

richardliaw May 10, 2018

richardliaw May 10, 2018

richardliaw May 15, 2018

alok commented May 10, 2018

AmplabJenkins commented May 10, 2018

richardliaw commented May 11, 2018

alok commented May 11, 2018

AmplabJenkins commented May 11, 2018

richardliaw left a comment

richardliaw May 15, 2018

richardliaw May 15, 2018

alok commented May 16, 2018

alok commented May 16, 2018

AmplabJenkins commented May 16, 2018

AmplabJenkins commented May 16, 2018

richardliaw commented May 17, 2018

alok commented May 17, 2018

alok commented May 17, 2018

richardliaw commented May 17, 2018

alok commented May 17, 2018

[DO NOT MERGE] Rewrite YAPF scripts #1971

[DO NOT MERGE] Rewrite YAPF scripts #1971

Conversation

alok commented May 1, 2018

What do these changes do?

Related issue number

pcmoritz commented May 2, 2018

alok commented May 2, 2018

AmplabJenkins commented May 2, 2018

alok commented May 4, 2018 • edited Loading

AmplabJenkins commented May 8, 2018

AmplabJenkins commented May 9, 2018

AmplabJenkins commented May 9, 2018

richardliaw left a comment

Choose a reason for hiding this comment

richardliaw May 10, 2018

Choose a reason for hiding this comment

richardliaw May 15, 2018

Choose a reason for hiding this comment

richardliaw May 10, 2018

Choose a reason for hiding this comment

richardliaw May 10, 2018

Choose a reason for hiding this comment

richardliaw May 15, 2018

Choose a reason for hiding this comment

alok commented May 10, 2018

AmplabJenkins commented May 10, 2018

richardliaw commented May 11, 2018

alok commented May 11, 2018

AmplabJenkins commented May 11, 2018

richardliaw left a comment

Choose a reason for hiding this comment

richardliaw May 15, 2018

Choose a reason for hiding this comment

richardliaw May 15, 2018

Choose a reason for hiding this comment

alok commented May 16, 2018

alok commented May 16, 2018

AmplabJenkins commented May 16, 2018

AmplabJenkins commented May 16, 2018

richardliaw commented May 17, 2018

alok commented May 17, 2018

alok commented May 17, 2018

richardliaw commented May 17, 2018

alok commented May 17, 2018

alok commented May 4, 2018 •

edited

Loading