Add Black for linting backend code #2967

wchargin · 2019-11-23T00:21:07Z

Fixes #2962. See most recent commit message for script to generate
reformatting commits.

This includes a one-time pass of docformatter to fix up docstrings,
which Black doesn’t reindent (because that would change the AST). This
was run at:

$ pip freeze | grep -e black -e docformatter
black==19.10b0
docformatter==1.3.1

googlebot · 2019-11-23T00:21:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

wchargin · 2019-11-23T00:22:02Z

CLAs are fine because I wrote or generated all of these commits.

The author is intentionally tensorboard-gardener@google.com.

googlebot · 2019-11-23T00:22:08Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-11-23T00:28:25Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

wchargin · 2019-11-23T05:26:30Z

Test CI runs can be found here:
https://www.travis-ci.org/wchargin/tensorboard/builds/615841246

(The wide range of this PR makes merge conflicts inevitable; I’ll grab a
merge lock when we’re ready to merge this.)

nfelt

🎉 Now that we have broad autoformatter coverage (and corresponding checks), I wonder if it's worth a mention in CONTRIBUTING.md or DEVELOPMENT.md just so that people don't have to wait until opening a PR to discover that we enforce these?

nfelt · 2019-12-11T19:47:29Z

tensorboard/plugins/scalar/scalars_plugin.py

+            db = self._db_connection_provider()
+            cursor = db.execute(
+                """
        SELECT


Black (understandably) doesn't fix the indentation here within the multiline string - we might want to do a manual pass over these to fix them up?

SGTM. I’d prefer to do this in a change separate to the autogenerated
reformatting. I can do all the DB cursor ones by hand, and maybe wrangle
up a tiny Awk script to find others.

Okay, here’s a script to find offenders (needs Python 3.8 for recent
changes to the ast module):

import ast import glob import os def main(): for filename in glob.glob("tensorboard/**/*.py", recursive=True): with open(filename) as infile: source = infile.read() sourcelines = source.splitlines() module = ast.parse(source, filename=filename) strings = [x for x in ast.walk(module) if isinstance(x, ast.Str)] for node in strings: base = _indent_of(sourcelines[node.lineno - 1]) lines = node.s.splitlines() if len(lines) <= 1: continue if any(_indent_of(x) < base for x in lines[1:] if x.strip()): print("%s:%d" % (filename, node.lineno)) def _indent_of(line): n = 0 for c in line: if c != " ": break n += 1 return n if __name__ == "__main__": main()

We have 235 such string literals across lots of files. With a quick spot
check, looks like we have:

True positives:

SQL queries

pbtxt literals

a few docstrings that docformatter didn’t fix because they weren’t well-formatted in the first place

False positives:

Flags in core_plugin.py, which have always been at column 0

Some strings with \n sequences in them, which ast doesn’t
distinguish from multi-line strings with newlines

I can probably just take a pass through all of these and fix them. There
aren’t that many, but they’ll require manual inspection to make sure
that the change is sound.

Filed #3034.

nfelt · 2019-12-11T20:10:34Z

tensorboard/compat/tensorflow_stub/pywrap_tensorflow.py

-    0x34f4f86a, 0xc69f7b69, 0xd5cf889d, 0x27a40b9e,
-    0x79b737ba, 0x8bdcb4b9, 0x988c474d, 0x6ae7c44e,
-    0xbe2da0a5, 0x4c4623a6, 0x5f16d052, 0xad7d5351,
+    0x00000000,


Maybe worth a # fmt: off for this block to keep this collapsed 4 to a line? The contents seem pretty unlikely to change.

Fine with me. With fmt: off, Black still adjusts the indentation,
but leaves the constants at 4-per-line, which seems perfect. (edit: It
doesn’t touch the indentation, either; this file is just already 4-space
indented.) It also doesn’t upcase them, so I’ll make that change
manually for consistency.

Done in #3023.

nfelt · 2019-12-11T20:17:02Z

tensorboard/compat/__init__.py

-      return tensorflow
+        from tensorboard.compat import (
+            notf,
+        )  # pylint: disable=g-import-not-at-top


I think reflowing this line comment onto its own line probably renders it inoperative (and it wasn't terribly useful anyway other than suppressing errors when developing internally). If we remove it, does Black not do the weird parenthesized import? (We could remove the bad instances of too-long line comments preemptively in a separate PR.)

Yeah. I suspect that the vast majority of our pylint comments are
useless, given that we basically only lint for syntax errors and free
variables. For instance, we have line-too-long on imports, but Google
style says that imports are unbounded-length, anyway, so that should be
superfluous.

I’ll go ahead and remove line-too-long and import-not-at-tops.

Done in #3021 and #3022, which does resolve this.

wchargin

I wonder if it's worth a mention in CONTRIBUTING.md or DEVELOPMENT.md
just so that people don't have to wait until opening a PR to discover
that we enforce these?

Yep! (That’s listed in the plan in #2962: “Update CONTRIBUTING.md to
show how to configure format-on-save.”)

Thanks for the comments!

wchargin · 2019-12-11T22:05:38Z

tensorboard/plugins/scalar/scalars_plugin.py

+            db = self._db_connection_provider()
+            cursor = db.execute(
+                """
        SELECT


SGTM. I’d prefer to do this in a change separate to the autogenerated
reformatting. I can do all the DB cursor ones by hand, and maybe wrangle
up a tiny Awk script to find others.

wchargin · 2019-12-11T22:09:50Z

tensorboard/compat/tensorflow_stub/pywrap_tensorflow.py

-    0x34f4f86a, 0xc69f7b69, 0xd5cf889d, 0x27a40b9e,
-    0x79b737ba, 0x8bdcb4b9, 0x988c474d, 0x6ae7c44e,
-    0xbe2da0a5, 0x4c4623a6, 0x5f16d052, 0xad7d5351,
+    0x00000000,


Fine with me. With fmt: off, Black still adjusts the indentation,
but leaves the constants at 4-per-line, which seems perfect. (edit: It
doesn’t touch the indentation, either; this file is just already 4-space
indented.) It also doesn’t upcase them, so I’ll make that change
manually for consistency.

wchargin · 2019-12-11T22:13:28Z

tensorboard/compat/__init__.py

-      return tensorflow
+        from tensorboard.compat import (
+            notf,
+        )  # pylint: disable=g-import-not-at-top


Yeah. I suspect that the vast majority of our pylint comments are
useless, given that we basically only lint for syntax errors and free
variables. For instance, we have line-too-long on imports, but Google
style says that imports are unbounded-length, anyway, so that should be
superfluous.

I’ll go ahead and remove line-too-long and import-not-at-tops.

@nfelt

Summary: Per suggestion of @nfelt on #2967. We don’t use Pylint for line length, and Google style says that imports are exempt from line length limits, anyway. Generated with: ``` git ls-files -z '*.py' | xargs -0 sed -i -e '/from.*import/s@ *# pylint: disable=line-too-long$@@' ``` Test Plan: Our standard lint still passes: ``` $ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics 0 ``` wchargin-branch: unsuppress-long-imports

@nfelt

Summary: Per suggestion of @nfelt on #2967. We don’t use Pylint for line length, and Google style says that imports are exempt from line length limits, anyway. Generated with: ``` git ls-files -z '*.py' | xargs -0 sed -i -e '/from.*import/s@ *# pylint: disable=line-too-long$@@' ``` Test Plan: Our standard lint still passes: ``` $ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics 0 ``` wchargin-branch: unsuppress-long-imports

@nfelt

Summary: Per suggestion of @nfelt on #2967, we abstain from reformatting the CRC table to one-per-line, for brevity and because this table is unlikely to experience churn. We manually upcase the constants for consistency with Black style. Test Plan: Running `black` on this file no longer changes the CRC table. wchargin-branch: fmtoff-crc-table

@nfelt

Summary: Per suggestion of @nfelt on #2967, we abstain from reformatting the CRC table to one-per-line, for brevity and because this table is unlikely to experience churn. We manually upcase the constants for consistency with Black style. Test Plan: Running `black` on this file no longer changes the CRC table. wchargin-branch: fmtoff-crc-table

Summary: Black doesn’t have many configuration options. The default line length is 88, but we’ll use 80 for consistency with Google style. By default, Black attempts to automatically detect the Python version on a per-file basis; we explicitly specify that we require Python 2.7 compatibility to avoid any surprises. Test Plan: Running `black --check .` successfully runs Black (with lots of failures, of course). wchargin-branch: pyproject-toml

…tensorboard_plugin_example

Summary: The Black binary only runs Python 3.6 and up, so we only lint in the Python 3 CI build, but that’s perfectly fine. I’ve moved that build to be first so that we can fail fast. The preceding large sequence of change commits was generated with: ``` #!/bin/sh set -eu black . docformatter -i -r . black . # should be a no-op export GIT_AUTHOR_NAME="TensorBoard Gardener" export GIT_AUTHOR_EMAIL="tensorboard-gardener@google.com" # Commit each directory in order of longest-named directory to # shortest-named, to force a topological ordering. Collapse some select # subtrees to reduce the number of commits. git diff --numstat \ | awk '{ print $3 }' \ | rev \ | cut -d / -f 2- \ | rev \ | sed -e 's#/$test\|demo$$##' \ | sed -e 's#^$tensorboard/compat$/.*#\1#' \ | sed -e 's#^$tensorboard/plugins/interactive_inference$/.*#\1#' \ | sed -e 's#^$tensorboard/summary$/.*#\1#' \ | sort -u \ | awk '{ print length($0), $0 }' \ | sort -nrs \ | awk '{ print $2 }' \ | while read -r dir do git add "${dir}" if git commit -m "black: reformat directory ${dir}" >/dev/null 2>&1; then printf 'reformat %s: ' "${dir}" git show --oneline --shortstat | tail -n 1 else printf 'skip %s (covered by subtrees)\n' "${dir}" fi done ``` Test Plan: Running `black --check .` currently passes, as all files have been fixed up. It takes about 0.3 seconds on my machine. wchargin-branch: ci-black

googlebot · 2019-12-14T19:51:59Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Summary: As of #2967, backend code is linted with Black. Test Plan: One can successfully install and run the listed version of Black: ``` ve="$(mktemp -d)" && virtualenv -q -p python3.7 "${ve}" && . "${ve}/bin/activate" && pip install black==19.10b0 && black . ``` This prints, “All done! 343 files left unchanged.” wchargin-branch: docs-black

Summary: As of #2967, backend code is linted with Black. Test Plan: One can successfully install and run the listed version of Black: ``` ve="$(mktemp -d)" && virtualenv -q -p python3.7 "${ve}" && . "${ve}/bin/activate" && pip install tf-nightly \ -r tensorboard/pip_package/requirements.txt \ -r tensorboard/pip_package/requirements_dev.txt \ && black . ``` This prints, “All done! 347 files left unchanged.” wchargin-branch: docs-black

wchargin added type:cleanup cla: yes labels Nov 23, 2019

googlebot added cla: no and removed cla: yes labels Nov 23, 2019

wchargin added cla: yes and removed cla: no labels Nov 23, 2019

wchargin force-pushed the wchargin-ci-black branch from 4bc0621 to 8f176a2 Compare November 23, 2019 00:27

googlebot added cla: no and removed cla: yes labels Nov 23, 2019

wchargin mentioned this pull request Nov 23, 2019

[test] Add Black for linting backend code wchargin/tensorboard#4

Open

wchargin mentioned this pull request Dec 11, 2019

Use Black #2962

Closed

3 tasks

nfelt approved these changes Dec 11, 2019

View reviewed changes

wchargin commented Dec 11, 2019

View reviewed changes

wchargin mentioned this pull request Dec 11, 2019

pylint: remove long-line suppressions on imports #3021

Merged

wchargin mentioned this pull request Dec 11, 2019

black: manually format CRC table #3023

Merged

wchargin and others added 6 commits December 14, 2019 11:43

black: reformat directory tensorboard/examples/plugins/example_basic/…

b8027f5

…tensorboard_plugin_example

black: reformat directory tensorboard/examples/plugins/example_basic

2556bc8

black: reformat directory tensorboard/plugins/interactive_inference

bc9512e

black: reformat directory tensorboard/backend/event_processing

85fdd5b

black: reformat directory tensorboard/plugins/custom_scalar

83bd6c7

tensorboard-gardener and others added 17 commits December 14, 2019 11:45

black: reformat directory tensorboard/plugins/image

e734864

black: reformat directory tensorboard/plugins/core

867fe60

black: reformat directory tensorboard/plugins/mesh

36d3780

black: reformat directory tensorboard/plugins/text

31f7f76

black: reformat directory tensorboard/pip_package

dd938d9

black: reformat directory tensorboard/uploader

cf82e96

black: reformat directory tensorboard/backend

0e83331

black: reformat directory tensorboard/plugins

6242071

black: reformat directory tensorboard/scripts

81bac83

black: reformat directory tensorboard/summary

f5fdf79

black: reformat directory tensorboard/compat

c89b02b

black: reformat directory tensorboard/tools

b311a64

black: reformat directory tensorboard/data

436ce40

black: reformat directory tensorboard/defs

5485dd9

black: reformat directory tensorboard/util

e694e4d

black: reformat directory tensorboard

4070c2a

wchargin force-pushed the wchargin-ci-black branch from 8f176a2 to abd01e8 Compare December 14, 2019 19:50

wchargin changed the title ~~[draft] Add Black for linting backend code~~ Add Black for linting backend code Dec 14, 2019

wchargin added cla: yes and removed cla: no labels Dec 14, 2019

wchargin merged commit 74214c3 into master Dec 14, 2019

wchargin deleted the wchargin-ci-black branch December 14, 2019 20:28

This was referenced Dec 14, 2019

docs: add instructions for linting Python code #3033

Merged

Clean up badly-dedented strings #3034

Closed

wchargin mentioned this pull request Sep 17, 2020

[oss-timeseries] migrate metrics backend #4168

Merged

Add Black for linting backend code #2967

Add Black for linting backend code #2967

Uh oh!

Conversation

wchargin commented Nov 23, 2019

Uh oh!

googlebot commented Nov 23, 2019

Uh oh!

wchargin commented Nov 23, 2019

Uh oh!

googlebot commented Nov 23, 2019

Uh oh!

googlebot commented Nov 23, 2019

Uh oh!

wchargin commented Nov 23, 2019

Uh oh!

nfelt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchargin Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchargin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchargin Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Dec 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wchargin Dec 11, 2019 •

edited

Loading

wchargin Dec 11, 2019 •

edited

Loading