Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC] Add Python code formatting with autopep8 #3298

Closed
wants to merge 1 commit into from

Conversation

@SplitInfinity
Copy link
Contributor

commented Jul 25, 2019

Summary
This commit adds automatic formatting of Python code in utils and
torch_glow (just the tests) using autopep8, a Python autoformatting tool.
README.md has also been updated to include installation instructions
for autopep8 via brew.

Test Plan
This commit also includes the files modified by running the updated
version of utils/format.sh.

Documentation
README.md has also been updated to include installation instructions for autopep8 via brew.

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

The only changes that reviewers should focus on are in utils/format.sh. The rest are all changes to the Python files in the project made by running the new version of this script.

@SplitInfinity SplitInfinity marked this pull request as ready for review Jul 25, 2019

@bertmaher
Copy link
Contributor

left a comment

So clean :-). While you're doing this, though, what do you think about using black? I think that's been recommended inside FB (and anecdotally I kind of love it). I'm fine either way if you feel strongly though.

@jackm321

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Amazing, looks so much better.
I hadn't heard of black but it does look pretty good and if it's used widely internally maybe it's best to use it.

@SplitInfinity SplitInfinity force-pushed the SplitInfinity:add-autopep8 branch from 7bba30e to be5835f Jul 25, 2019

@SplitInfinity SplitInfinity changed the title [NFC] Add Python code formatting with autopep8 [NFC] Add Python code formatting with black Jul 25, 2019

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Switched to black. I also had to migrate the print functions in utils/imagenet_topk_accuracy_driver.py from Python 2 style syntax to Python 3 style syntax to satisfy black, so watch out for that during review.

black needs Python 3.6+ to run, but it looks like our Circle CI uses an older version and so it can't install black and that's why all of the CI jobs have failed. @rdzhabarov, what is the right thing to do here?

@SplitInfinity SplitInfinity force-pushed the SplitInfinity:add-autopep8 branch from be5835f to 140cadd Jul 25, 2019

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Added -q flag (quiet) to black invocation so that it only prints messages when it encounters an error.

@bertmaher

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

It looks like .circleci/config.yml uses py3.6 for the pytorch tests. Can we use that everywhere?

@bertmaher

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Oh. clang8 vs clang7 might be a problem. Although we should still be compatible with 7, I thought...

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I formatted #3290 with black and flake8/autopep8 are not happy with the result. Phabricator doesn't like it either. Is it really the case that we use black internally? Is there a version difference, maybe?

@bertmaher

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Supposedly, but who knows. Let's just use autopep.

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

black --version shows black, version 19.3b0 on both my laptop and devserver, so that's not it. Okay, I'll convert back to autopep8.

@SplitInfinity SplitInfinity force-pushed the SplitInfinity:add-autopep8 branch from 140cadd to f43d977 Jul 25, 2019

@SplitInfinity SplitInfinity changed the title [NFC] Add Python code formatting with black [NFC] Add Python code formatting with autopep8 Jul 25, 2019

@SplitInfinity SplitInfinity force-pushed the SplitInfinity:add-autopep8 branch from f43d977 to 3a79022 Jul 25, 2019

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

So for some reason there is a timeout on CI for the CHECK_CLANG_FORMAT job, but the same command takes a few seconds on my laptop:

$ time ./utils/format.sh check
/usr/local/bin/clang-format
Running check format.

real	0m7.084s
user	0m39.274s
sys	0m1.988s

@rdzhabarov Any advice?

.circleci/build.sh Outdated Show resolved Hide resolved
utils/format.sh Outdated Show resolved Hide resolved
utils/format.sh Outdated Show resolved Hide resolved

@SplitInfinity SplitInfinity force-pushed the SplitInfinity:add-autopep8 branch from 3a79022 to 238db12 Jul 26, 2019

@bertmaher
Copy link
Contributor

left a comment

Running this on my Macbook looks like utils/compilation_filter.py triggers some kind of pathological behavior, because it's running for ages on that file.

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

That must be causing the CHECK_CLANG_FORMAT job timeout too :O

@bertmaher

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

It seems OK without the -as, maybe let's just do the non-aggressive version?

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@bertmaher Out of curiosity, which version of autopep8 do you have? It terminates on my machine.

$ autopep8 --version
autopep8 1.4.4 (pycodestyle: 2.5.0)
$ time autopep8 -i -a -a -j -1 utils/compilation_filter.py 

real	0m0.540s
user	0m0.448s
sys	0m0.072s

In any case, I'll remove the -a option and push again.

[NFC] Add Python code formatting with autopep8
Summary:
This commit adds automatic formatting of Python code in utils and
torch_glow (just the tests) using autopep8, a Python autoformatting tool.
README.md has also been updated to include installation instructions
for autopep8 via brew.

Test Plan:
This commit also includes the files modified by running the updated
version of utils/format.sh.

@SplitInfinity SplitInfinity force-pushed the SplitInfinity:add-autopep8 branch from 238db12 to b810082 Jul 26, 2019

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Removed -a options in an attempt to prevent CHECK_CLANG_FORMAT job from timing out.

@bertmaher

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Bizarre, I've the same thing:

autopep8 1.4.4 (pycodestyle: 2.5.0)
@bertmaher

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Uh, this is interesting. If I install with pip it hangs, with pip3.7 it's fast. Is the py2 version somehow borked?

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

That's a possibility - I got mine with pip3.7.

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

At last! I guess we'll settle for regular mode for now and turn on aggressive later if we need it.

@bertmaher
Copy link
Contributor

left a comment

Woohoo!
🚢🚣🚤🚀

@facebook-github-bot
Copy link

left a comment

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

@SplitInfinity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Phabricator is mostly happy. I'll land this.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jul 26, 2019

@SplitInfinity merged this pull request in 59f26ae.

@jackm321

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@SplitInfinity I don't know why but this doesn't work on my mac. I pip installed autopep8 (with python3) and ran utils/format.sh fix and it doesn't change anything (I don't think it's cause my Python is perfect already). When I feed files to autopep8 manually with the -d flag still it recommends no changes. Any ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.