Glint everything #3654

karmel · 2018-03-19T23:04:43Z

So much linting. Please don't introduce any new lint errors after this. Tests will be updated to check as well.

karmel · 2018-03-19T23:05:56Z

In the interest of sharing the pain-- everyone look over the files you touched recently.

karmel · 2018-03-19T23:07:45Z

official/utils/logging/hooks_helper.py

      LoggingTensorHook, ProfilerHook, ExamplesPerSecondHook, which are defined
      as keys in HOOKS
-    kwargs: a dictionary of arguments to the hooks.
+    **kwargs: a dictionary of arguments to the hooks.


This seems silly in the Args, but glint complains otherwise.

Yeah, I got the same warning when glinting it . Will follow this style later. LGTM, thanks.

k-w-w

Wow, that's a lot of lint. Thanks for cleaning all the code!

k-w-w · 2018-03-19T23:27:53Z

official/resnet/imagenet_preprocessing.py

-  `method` and other details each time.
+  """Simple wrapper around tf.resize_images.
+
+  This is primarily to make sure we use the same `method` and other details


nit: replace method with ResizeMethod

k-w-w · 2018-03-19T23:38:21Z

official/resnet/resnet_model.py

+    data_format: The input format ('channels_last' or 'channels_first').
+
+  Returns:
+    The output tensor of the block.


comment that the output shape should be the same as the inputs

asimshankar · 2018-03-19T23:51:15Z

official/mnist/dataset.py

  with tf.gfile.Open(filename, 'rb') as f:
    magic = read32(f)
-    num_images = read32(f)
+    num_images = read32(f)  # pylint: disable=unused-variable


Would it be better to write this as:

read32(f) # num_images, ignored

And same for num_items below?

yhliang2018

LGTM. Thanks!

robieta

LGTM.

qlzh727 · 2018-03-20T17:11:43Z

official/mnist/dataset.py

  with tf.gfile.Open(filename, 'rb') as f:
    magic = read32(f)
-    num_items = read32(f)
+    num_items = read32(f)  # pylint: disable=unused-variable


I think instead of put a suppression of lint here, you can just rename the variable as unused_num_items and I think it will make the code more readable, and also make the lint warning go away.

Solved slightly differently, as per @asimshankar 's comments above.

qlzh727 · 2018-03-20T17:13:04Z

official/mnist/mnist.py

+    ValueError: if no GPUs are found, or selected batch_size is invalid.
  """
-  from tensorflow.python.client import device_lib
+  from tensorflow.python.client import device_lib  # pylint: disable=g-import-not-at-top


Is there any reason that we have to do a local import and not put it on top?

We want to hide this code, as it should not be exposed to the user, and we want to remove it as soon as it's done by Estimator directly. So, for now, leaving bundled.

qlzh727 · 2018-03-20T17:23:17Z

official/mnist/mnist_eager.py

+from official.mnist import dataset as mnist_dataset
 from official.mnist import mnist
-from official.mnist import dataset



I think glint will give u a snippet to paste for incorrect order, should we do that instead of put a lint suppression here?

The problem is TF versus official; alphabetically TF should come after official, and to Google, their both the same, but for us, TF is third party.

k-w-w

Just one nit!

k-w-w · 2018-03-20T19:45:56Z

official/wide_deep/wide_deep.py


 _NUM_EXAMPLES = {
    'train': 32561,
    'validation': 16281,


nit: remove extra space

karmel · 2018-03-20T19:55:45Z

I added an rcfile that we will be able to run with pylint:

RCFILE="utils/testing/pylint.rcfile"

for file in `find . -name '*.py' ! -name '*test.py' -print`
do
  echo "Linting ${file}"
  pylint --rcfile=$RCFILE $file
done

# More lenient for test files.
for file in `find . -name '*test.py' -print`
do
  echo "Linting ${file}"
  pylint --rcfile=$RCFILE --disable=missing-docstring,protected-access $file
done

Glint everything

6c076e9

karmel requested review from k-w-w and nealwu as code owners March 19, 2018 23:04

googlebot added the cla: yes label Mar 19, 2018

karmel added the kokoro:force-run label Mar 19, 2018

kokoro-team removed the kokoro:force-run label Mar 19, 2018

karmel requested review from qlzh727, robieta and yhliang2018 March 19, 2018 23:05

karmel commented Mar 19, 2018

View reviewed changes

k-w-w approved these changes Mar 19, 2018

View reviewed changes

asimshankar reviewed Mar 19, 2018

View reviewed changes

yhliang2018 approved these changes Mar 20, 2018

View reviewed changes

robieta approved these changes Mar 20, 2018

View reviewed changes

qlzh727 reviewed Mar 20, 2018

View reviewed changes

karmel added 2 commits March 20, 2018 12:30

Adding rcfile and pylinting

a3c1d05

Fixing conflicts

d1d83ae

k-w-w reviewed Mar 20, 2018

View reviewed changes

official/wide_deep/wide_deep.py Outdated

_NUM_EXAMPLES = {

'train': 32561,

'validation': 16281,

Copy link

Contributor

k-w-w Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove extra space

Extra newline

4f57ba6

Few last lints

39e5e7b

karmel added the kokoro:force-run label Mar 20, 2018

kokoro-team removed the kokoro:force-run label Mar 20, 2018

karmel merged commit 7cfb6bb into master Mar 20, 2018

karmel deleted the fix/glint branch March 20, 2018 20:10

Glint everything #3654

Glint everything #3654

Uh oh!

Conversation

karmel commented Mar 19, 2018

Uh oh!

karmel commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-w-w 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!

yhliang2018 left a comment

Choose a reason for hiding this comment

Uh oh!

robieta 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!

Choose a reason for hiding this comment

Uh oh!

k-w-w left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karmel commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants