Move argparsing from builtin argparse to absl #4099

robieta · 2018-04-26T20:45:58Z

This is a mock up of what arg parsing would look like with absl.flags instead of argparse. Run demo.py to see -help and -helpfull. Sadly there is no -h.

EDIT: -h is possible.

qlzh727 · 2018-04-26T22:40:37Z

official/utils/flags/_base.py

+# limitations under the License.
+# ==============================================================================
+
+import absl.flags


I would do from absl import flags, so that all the code below can do flag.DEFINE_xxx. This will make the copybara rule easy, and also work with internal code.

Seems reasonable.

qlzh727 · 2018-04-26T22:56:39Z

official/utils/flags/core.py

+def define_base(*args, **kwargs):
+  key_flags = _base.define_base(*args, **kwargs)
+  [absl.flags.declare_key_flag(fl) for fl in key_flags]
+


Why do we have this extra level of define? Can't we just mark the flags to be key flag when they are declared?

If declared when flags.DEFINE_* is called then they are key flags only for that module. However the declaration needs to be in core.py in order for the adopt_module_key_flags() chaining to work correctly. DEFINEs are magical context-aware little beasts.

qlzh727 · 2018-04-26T23:01:29Z

official/utils/flags/demo.py

+import absl.app
+import absl.flags
+
+from official.utils.flags import core


I would put an alias here to core_flags, so that the core.define_base() becomes core_flags.define_base(), which provides more context about core package.

qlzh727 · 2018-04-26T23:01:56Z

official/utils/flags/demo.py

+# limitations under the License.
+# ==============================================================================
+
+import absl.app


same, from absl import app.

I am less sold on this one. Other things could conceivably want the namespace app.

as absl_app? Moot point if this will be removed.

karmel · 2018-04-27T17:09:26Z

official/utils/flags/_base.py

@@ -0,0 +1,102 @@
+# Copyright 2018 The TensorFlow Authors. All Rights Reserved.


Reminder: this will all have to be added to BUILD files.

karmel · 2018-04-27T17:11:31Z

official/utils/flags/_conventions.py

+
+help_wrap = functools.partial(flags.text_wrap, length=80, indent="  ",
+                              firstline_indent="\n")
+


All magic must be extensively commented.

karmel · 2018-04-27T17:13:00Z

official/utils/flags/_misc.py

+
+  if export_dir:
+    flags.DEFINE_string(
+        name="export_dir", short_name="ed", default=None,


This should be in base now, as of #4041

karmel · 2018-04-27T17:14:22Z

official/utils/flags/_performance.py

+            to_choices_str(DTYPE_MAP.keys()))))
+
+    @flags.validator(flag_name="dtype", message="Valid dtypes: {}"
+                     .format(to_choices_str(DTYPE_MAP.keys())))


Ack. This is way too much to pack into a single decorator call. At least extract the string formatting.

karmel · 2018-04-27T17:15:05Z

official/utils/flags/_performance.py

+            "for fp16 is 128 and 1 for all other dtypes."))
+
+    @flags.validator(flag_name="loss_scale", message="loss_scale should be a "
+                                                     "positive integer.")


Ditto. extract message to _loss_scale_msg or similar, for clarity.

karmel · 2018-04-27T17:17:10Z

official/utils/flags/core.py

+
+def define_performance(*args, **kwargs):
+  key_flags = _performance.define_performance(*args, **kwargs)
+  [flags.declare_key_flag(fl) for fl in key_flags]


We will want an instruction set somewhere-- if I want to add a new flag, what do I do, and what decorators/key_flags/etc. does it require? Similarly, for adding a new group of flags, and removing a flag or group.

karmel · 2018-04-27T17:17:28Z

official/utils/flags/demo.py

+# limitations under the License.
+# ==============================================================================
+
+import absl.app


as absl_app? Moot point if this will be removed.

karmel · 2018-04-27T17:20:07Z

official/utils/logs/hooks_helper.py

+    "p": "ProfilerHook",
+    "eps": "ExamplesPerSecondHook",
+    "lm": "LoggingMetricHook",
+}


Why are we relying more on abbreviations here? This makes me nervous, because it's easy to imagine someone adding a new flag-- say, --parse, -p, and then having sort out later why things break when the loggers are added too.

This is something I've wanted for a while, but the formatting in argparse forbade a good help message for it. I added an "h" to the end of them all, so ProfilerHook is "ph" instead of "p" to make it a little harder to mess up.

karmel · 2018-04-27T17:20:59Z

official/utils/flags/_base.py

+            "A comma separated list of (case insensitive) strings to specify "
+            "the names of training hooks.\n{}\n\u180E  "
+            "Example: `--hooks ProfilerHook,ExamplesPerSecondHook`\n\u180E  "
+            "(or)     `-hk p,eps`\nSee official.utils.logs.hooks_helper for "


What's going on with formatting here? Intentional?

Intentional. I go into more detail in flags/_example.py, but basically it is needed for pretty help messages.

karmel · 2018-04-27T17:21:55Z

official/utils/flags/_base.py

+    # Construct a pretty summary of hooks.
+    pad_len = max([len(i) for i in hooks_helper.HOOKS_ALIAS.values()]) + 6
+    hook_list_str = (
+        "\u180E  {}Abbreviation\n".format("Hook".ljust(pad_len)) + "\n".join(


I dislike having to encode unicode chars here. Are normal characters insufficient? A dash or asterisk wouldn't suffice?

I share your trepidation, but I've tested in P2 and P3. There sadly isn't a good ASCII solution.

robieta · 2018-04-30T20:47:40Z

@karmel @qlzh727 This is ready for a second pass.

karmel

High-level question: should we be using tf.flags to keep consistent with tf proper? https://github.com/tensorflow/tensorflow/blob/r1.8/tensorflow/python/platform/flags.py

karmel · 2018-05-01T00:14:23Z

official/mnist/mnist.py

      ])


+@flags_core.call_only_once


Nooooo. You can't use absl AND arg parsing magic.

second it. If we do this just because of test case, then we are doing something wrong in test.

Grumble grumble. Actually the setupClass approach is much better. I'm happy to ditch this decorator.

karmel · 2018-05-01T00:15:57Z

official/mnist/mnist.py


-  if flags.multi_gpu:
-    validate_batch_size_for_multi_gpu(flags.batch_size)
+  if FLAGS.multi_gpu:


One benefit of having the flags passed in is that it's easier to use/reuse main by passing in an arbitrary object. Thoughts on keeping this as main(flags) and passing in FLAGS in main?

The FLAGS value can actually be overwritten, eg by test class. I was lying about the immutability for the flag values within FLAGS instance. The one in JAVA is immutable, and the python one is mutable.

Initially I directly used the global because of the need to make calls to things like get_tf_dtype() for values which aren't stored directly in the FLAGS object, and it seemed wrong to take a parameter and then deeper in call out to a global. I think I'm just going to make all conversion functions pure functions to address that.

I'm still very much of the opinion that a global parameter object is something that is tolerated rather than desired, so having a clear and early conversion into more normal OOP is very appealing.

karmel · 2018-05-01T00:18:26Z

official/mnist/mnist_eager.py

  # Automatically determine device and data_format
  (device, data_format) = ('/gpu:0', 'channels_first')
-  if flags.no_gpu or tfe.num_gpus() <= 0:
+  if flags.FLAGS.no_gpu or tfe.num_gpus() <= 0:


Why FLAGS in mnist, but flags.FLAGS here?

Agreed, should just be consistent and stay with FLAGS.

karmel · 2018-05-01T00:22:05Z

official/resnet/resnet_run_loop.py

+  def _check_version(v):
+    return v in choices
+
+  rs_choice_str = ""


nit: Different quotations marks used.

karmel · 2018-05-01T00:23:09Z

official/resnet/resnet_run_loop.py

+
+  rs_choice_str = ""
+  if resnet_size_choices is not None:
+    rs_choice_str = '\n' + flags_core.to_choices_str(resnet_size_choices)


Is this just to add a newline I would prefer not having a newline and removing 4 lines of argparsing code.

karmel · 2018-05-01T02:00:09Z

official/utils/flags/_example.py

+from __future__ import division
+from __future__ import print_function
+
+from absl import flags


This is cute, but I would prefer a guide (md) that starts with use cases and then gives examples. As a developer, I want a simple "Here's how to add a new flag, here's how to add a new set of flags, here's how to validate a flag," rather than having to find and parse a separate module.

Please don't call my work cute. I've added a readme that outlines the basics of using absl, although I don't want to replicate too much of absl's docs.

karmel · 2018-05-01T02:04:57Z

official/mnist/mnist_eager.py

+def define_mnist_eager_flags():
+  """Defined flags and defaults for MNIST in eager mode."""
+  flags_core.define_base_eager()
+  flags_core.define_image()


image_model might be more clear?

I think it is less clear. (Since we're not actually defining any part of the model itself.)

karmel · 2018-05-01T02:32:34Z

official/resnet/resnet_run_loop.py

+  )
+  @flags.validator('version', message='Invalid ResNet version.')
+  def _check_version(v):
+    return v in choices


Does absl really not validate natively that the chosen choice is in choices? That is surprising.

Using enum.

karmel · 2018-05-01T02:33:42Z

official/mnist/mnist_eager.py

+  """Defined flags and defaults for MNIST in eager mode."""
+  flags_core.define_base_eager()
+  flags_core.define_image()
+  flags.adopt_module_key_flags(flags_core)


I've read through flags_core, and I still don't understand what this is doing. What does this do, and can we somehow make that activity more clear?

It makes arguments show up in -help instead of just -helpfull. I think there is just going to have to be some familiarizing with absl concepts like key flags.

karmel · 2018-05-01T02:35:26Z

official/utils/flags/demo.py

+
+if __name__ == "__main__":
+  define_flags()
+  absl_app.run(main)


This should be included in docs potentially, but not as a first-class module.

qlzh727 · 2018-05-01T14:33:45Z

official/mnist/mnist.py


-  if flags.multi_gpu:
-    validate_batch_size_for_multi_gpu(flags.batch_size)
+  if FLAGS.multi_gpu:


The FLAGS value can actually be overwritten, eg by test class. I was lying about the immutability for the flag values within FLAGS instance. The one in JAVA is immutable, and the python one is mutable.

qlzh727 · 2018-05-01T14:37:48Z

official/mnist/mnist.py

      ])


+@flags_core.call_only_once


second it. If we do this just because of test case, then we are doing something wrong in test.

qlzh727 · 2018-05-01T14:40:02Z

official/mnist/mnist_eager.py

  # Automatically determine device and data_format
  (device, data_format) = ('/gpu:0', 'channels_first')
-  if flags.no_gpu or tfe.num_gpus() <= 0:
+  if flags.FLAGS.no_gpu or tfe.num_gpus() <= 0:


Agreed, should just be consistent and stay with FLAGS.

qlzh727 · 2018-05-01T14:42:33Z

official/resnet/cifar10_main.py


-  input_function = flags.use_synthetic_data and get_synth_input_fn() or input_fn
+def main(_):
+  input_function = (flags.FLAGS.use_synthetic_data and get_synth_input_fn()


Same, please use FLAGS instead of flags.FLAGS.

please fix.

qlzh727 · 2018-05-01T14:44:02Z

official/resnet/cifar10_test.py

    self._test_cifar10model_shape(version=2)

  def test_cifar10_end_to_end_synthetic_v1(self):
+    cifar10_main.define_cifar_flags()


Since we are not statically defining flags, we will then need a static setup function in the test case to cover that.

qlzh727 · 2018-05-01T15:28:50Z

official/utils/flags/_conventions.py

+
+
+def to_choices_str(choices):
+  return "(choices: {})".format(", ".join([str(i) for i in choices]))


The enum flag can support this I think

qlzh727 · 2018-05-01T15:31:11Z

official/utils/flags/_misc.py

+
+  if data_format:
+    choices = ["channels_first", "channels_last"]
+    flags.DEFINE_string(


Just use the enum flag and validation is come by default.

qlzh727 · 2018-05-01T15:31:52Z

official/utils/flags/_performance.py

+
+
+def get_tf_dtype():
+  return DTYPE_MAP[flags.FLAGS.dtype][0]


Please use FLAGS instead of flags.FLAGS.

qlzh727 · 2018-05-01T15:36:13Z

official/utils/flags/core.py

+
+def call_only_once(f):
+  """Prevent unittests from defining flags multiple times."""
+  setattr(f, "already_called", False)


Please use the static setup function in test class to ensure the flags is defined only once.

qlzh727 · 2018-05-01T15:41:05Z

official/utils/flags/flags_test.py

 class BaseTester(unittest.TestCase):

+  def setUp(self):
+    define_flags()


Should do as following:

@classmethod
def setUpClass(cls):
define_flags()

qlzh727 · 2018-05-01T22:03:37Z

official/mnist/mnist.py

-  parser = MNISTArgParser()
-  flags = parser.parse_args(args=argv[1:])
-
+def main(_):


instead of just _ here, which might confuse people, how about "unused_argv"?

qlzh727 · 2018-05-01T22:08:02Z

official/resnet/cifar10_main.py


-  input_function = flags.use_synthetic_data and get_synth_input_fn() or input_fn
+def main(_):
+  input_function = (flags.FLAGS.use_synthetic_data and get_synth_input_fn()


please fix.

qlzh727 · 2018-05-01T22:12:50Z

official/utils/flags/core.py

+    flags.FLAGS.set_default(name=key, value=value)
+
+
+def parse_flags(argv=None):


This should be removed now.

I don't think so. It's still used.

robieta

I have addressed the comments; however, I need to rewrite history (which will do who knows what to reviews on github) so I'm submitting this review now. I will make another comment when you should actually review it.

robieta · 2018-05-01T22:48:50Z

official/mnist/mnist.py

-  parser = MNISTArgParser()
-  flags = parser.parse_args(args=argv[1:])
-
+def main(_):


robieta · 2018-05-01T22:53:02Z

official/mnist/mnist.py


-  if flags.multi_gpu:
-    validate_batch_size_for_multi_gpu(flags.batch_size)
+  if FLAGS.multi_gpu:


Initially I directly used the global because of the need to make calls to things like get_tf_dtype() for values which aren't stored directly in the FLAGS object, and it seemed wrong to take a parameter and then deeper in call out to a global. I think I'm just going to make all conversion functions pure functions to address that.

I'm still very much of the opinion that a global parameter object is something that is tolerated rather than desired, so having a clear and early conversion into more normal OOP is very appealing.

robieta · 2018-05-01T23:29:19Z

official/mnist/mnist_eager.py

  # Automatically determine device and data_format
  (device, data_format) = ('/gpu:0', 'channels_first')
-  if flags.no_gpu or tfe.num_gpus() <= 0:
+  if flags.FLAGS.no_gpu or tfe.num_gpus() <= 0:


robieta · 2018-05-01T23:30:14Z

official/mnist/mnist_eager.py

+def define_mnist_eager_flags():
+  """Defined flags and defaults for MNIST in eager mode."""
+  flags_core.define_base_eager()
+  flags_core.define_image()


I think it is less clear. (Since we're not actually defining any part of the model itself.)

robieta · 2018-05-01T23:34:18Z

official/mnist/mnist_eager.py

+  """Defined flags and defaults for MNIST in eager mode."""
+  flags_core.define_base_eager()
+  flags_core.define_image()
+  flags.adopt_module_key_flags(flags_core)


It makes arguments show up in -help instead of just -helpfull. I think there is just going to have to be some familiarizing with absl concepts like key flags.

robieta · 2018-05-02T16:24:48Z

official/utils/flags/_example.py

+from __future__ import division
+from __future__ import print_function
+
+from absl import flags


Please don't call my work cute. I've added a readme that outlines the basics of using absl, although I don't want to replicate too much of absl's docs.

robieta · 2018-05-02T16:30:04Z

official/utils/flags/core.py

+    flags.FLAGS.set_default(name=key, value=value)
+
+
+def parse_flags(argv=None):


I don't think so. It's still used.

robieta · 2018-05-02T16:38:10Z

official/utils/flags/core.py

+
+
+def parse_flags(argv=None):
+  """Reset flags and reparse. Currently only used in testing."""


I put it here because there isn't a utils/testing/misc.py, and this doesn't seem important enough to make one. (Plus we could end up using it somewhere else.)

robieta · 2018-05-02T17:09:38Z

official/utils/flags/flags_test.py

+          [__file__, "--dtype", dtype_str, "--loss_scale", "5"])

-      assert args.loss_scale == 5
+      assert flags_core.get_loss_scale() == 5


It should be. Fixed.

robieta · 2018-05-02T17:09:53Z

official/utils/logs/hooks_helper.py

+    "ph": "ProfilerHook",
+    "epsh": "ExamplesPerSecondHook",
+    "lmh": "LoggingMetricHook",
+}


Alright. They're gone.

karmel · 2018-05-02T22:55:29Z

official/resnet/resnet_run_loop.py

  Args:
-    flags: FLAGS object that contains the params for running. See
-      ResnetArgParser for created flags.
+    flags_obj: An object containing parsed flags.


nit: "See ... for details."

karmel · 2018-05-02T22:59:35Z

official/utils/flags/README.md

+
+## Key Flags
+absl has the concept of a key flag. Any flag defined in `__main__` is considered a key flag by
+default. Key flags are displayed in `--help`, others only appear in `--helpfull`. Obviously this


nit: it is not obvious that this is insufficient, or what precisely you mean here. Maybe you mean, "In order to handle key flags that are defined outside the module in question, absl provides..."

pro-tip: almost nothing is obvious when you're this deep into TF and other magical libraries.

karmel · 2018-05-02T23:04:53Z

official/utils/flags/README.md

+
+when `my_module.py` is run it will show the help text for `my_flag`. Because not all flags defined
+in a file are equally important, `official/utils/flags/core.py` provides an abstraction for handling
+key flag declaration in an easy way.


Namely? Confusing to mention but not show. Also, linking directly to the code in question is helpful.

karmel · 2018-05-02T23:06:54Z

official/utils/flags/README.md

+```
+
+Validators take the form that returning True (truthy) passes, and all others 
+(False, None, exception) fail.


Also: We provide common flags in .... To use these, call ... in your module.
And: In order to add a test for a module declaring flags, you must...

karmel · 2018-05-02T23:07:53Z

official/utils/flags/_conventions.py

+# line after flags are listed.
+help_wrap = functools.partial(flags.text_wrap, length=80, indent="",
+                              firstline_indent="\n")
+


Currently the only major effect? Is it expected that will change over time?

And, if this is staying, please doc it. How does one use this? When should one use this?

robieta · 2018-05-02T23:43:31Z

Addressed.

…rated lists is not as expected

* squash of modular absl usage commits * delint * address PR comments * change hooks to comma separated list, as absl behavior for space separated lists is not as expected

robieta requested review from karmel and qlzh727 April 26, 2018 20:45

robieta requested a review from a team as a code owner April 26, 2018 20:45

googlebot added the cla: yes label Apr 26, 2018

qlzh727 requested changes Apr 26, 2018

View reviewed changes

karmel suggested changes Apr 27, 2018

View reviewed changes

robieta force-pushed the absl_flag_proposal branch 2 times, most recently from a5d829c to 09a74f5 Compare April 30, 2018 18:09

robieta changed the title ~~WIP: Strawman for modular absl usage~~ Move argparsing from builtin argparse to absl Apr 30, 2018

karmel suggested changes May 1, 2018

View reviewed changes

qlzh727 requested changes May 1, 2018

View reviewed changes

robieta added the kokoro:force-run label May 2, 2018

kokoro-team removed the kokoro:force-run label May 2, 2018

robieta commented May 2, 2018

View reviewed changes

robieta force-pushed the absl_flag_proposal branch from ec3bd95 to 638530f Compare May 2, 2018 18:06

qlzh727 approved these changes May 2, 2018

View reviewed changes

Taylor Robie added 2 commits May 2, 2018 15:37

squash of modular absl usage commits

891d9da

delint

f53c230

robieta force-pushed the absl_flag_proposal branch from 90d69af to f53c230 Compare May 2, 2018 22:37

karmel suggested changes May 2, 2018

View reviewed changes

address PR comments

f7c5adc

karmel approved these changes May 3, 2018

View reviewed changes

change hooks to comma separated list, as absl behavior for space sepa…

a14664b

…rated lists is not as expected

robieta merged commit 5f9f6b8 into master May 3, 2018

robieta deleted the absl_flag_proposal branch May 3, 2018 00:25

		@@ -0,0 +1,102 @@
		# Copyright 2018 The TensorFlow Authors. All Rights Reserved.


		help_wrap = functools.partial(flags.text_wrap, length=80, indent=" ",
		firstline_indent="\n")



		def to_choices_str(choices):
		return "(choices: {})".format(", ".join([str(i) for i in choices]))

		flags.FLAGS.set_default(name=key, value=value)


		def parse_flags(argv=None):



		def parse_flags(argv=None):
		"""Reset flags and reparse. Currently only used in testing."""

		])


		@flags_core.call_only_once

		])


		@flags_core.call_only_once

Move argparsing from builtin argparse to absl #4099

Move argparsing from builtin argparse to absl #4099

Uh oh!

Conversation

robieta commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robieta commented Apr 30, 2018

Uh oh!

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

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!

robieta commented Apr 26, 2018 •

edited

Loading