Unified arg parser #3574

robieta · 2018-03-12T20:51:45Z

This PR breaks various groups of common arguments into their own argparse class to enable some degree of standardization among the official models. The resnet argparser is replaced as an example.

karmel

This is great-- really looking forward to having fewer instances of --multi_gpu defined across the code. Some comments toward simplification, but looking good.

karmel · 2018-03-13T16:24:53Z

official/__init__.py

@@ -0,0 +1,14 @@
+# Copyright 2017 The TensorFlow Authors. All Rights Reserved.


I don't think it matters one way or the other, but a quick check of TF proper implies that they only include the license in init files if there is actual code in them, otherwise empty. Fine to leave empty in this case, probably.

karmel · 2018-03-13T16:27:39Z

official/utils/arg_parsers/__init__.py

+# limitations under the License.
+# ==============================================================================
+
+from .parsers import LocationParser, DeviceParser, SupervisedParser, \


Let's stick with absolute imports. Both for pep8 and for the sake of rules that convert this code internally.

Sure. Fixed.

karmel · 2018-03-13T16:31:13Z

official/utils/arg_parsers/parsers.py

+"""
+
+import argparse
+


I think we can collapse some of these categories-- all will need location, device, supervised-- those can be lumped into ModelParser or something, with all args True by default. Here we have tiny groups, and also the ability to turn things on and off one by one, which seems redundant. If we collapse those three, then remove the need to turn each option on individually, you substantially reduce the overall code required in resnet above, for example.

Done. I left the default as False for the secondary classes.

Let's go ahead and make default=True for those as well. I would prefer to reduce the total amount of arg_parser code that users have to read through when looking at Resnet.

karmel · 2018-03-13T16:32:39Z

official/utils/arg_parsers/parsers.py

+
+    if data_dir:
+      self.add_argument(
+          "--data_dir", "-dd", default="/tmp",


Maybe /tmp/model-data or something like that? Seems like just /tmp will never be a good guess. Not that model-data is though. So maybe just an empty string? Ditto for model dir below.

I'm expecting defaults to be overridden. I am inclined to either use "/tmp" or an explicitly invalid string like "data_directory_path"

karmel · 2018-03-13T16:33:17Z

official/utils/arg_parsers/parsers.py

+    if data_dir:
+      self.add_argument(
+          "--data_dir", "-dd", default="/tmp",
+          help="[default: %(default)s] The location of the input data.",


Maybe comment on some of the arg parser magic here? What is the default templating, and metavar?

Added to the docstring.

karmel · 2018-03-13T16:35:57Z

official/utils/arg_parsers/parsers.py

+
+    if inter_op:
+      self.add_argument(
+          "--inter_op_parallelism_threads", "-inrt",


inrt is really hard for me to parse as an abbreviation. Maybe just inter/intra for these.

karmel · 2018-03-13T16:36:28Z

official/utils/arg_parsers/parsers.py

+
+
+class DummyParser(argparse.ArgumentParser):
+  """Default parser for specification of dummy/mocked behavior.


This should be combined with performance above.

karmel · 2018-03-13T16:36:51Z

official/utils/arg_parsers/parsers.py

+
+    if use_synthetic_data:
+      self.add_argument(
+          "--use_synthetic_data", "-usd",


karmel · 2018-03-13T16:37:24Z

official/utils/arg_parsers/parsers.py

+      )
+
+
+


Nit: too many \ns at the end here.

karmel · 2018-03-13T16:40:39Z

official/resnet/resnet.py

-        'for generally homogeneous data sets, should be approximately the '
-        'number of available CPU cores.')
+    super(ResnetArgParser, self).__init__(parents=[
+      official.utils.arg_parsers.LocationParser(data_dir=True, model_dir=True),


The import logic seems off here-- shouldn't this be from official.utils import arg_parsers above, then just arg_parsers.... here?

Yes, the import logic of official module is like @karmel mentioned.

Hm, it looks like from foo.bar import baz is allowed by the style guide. I will certainly change this.

qlzh727 · 2018-03-13T16:31:11Z

official/resnet/resnet.py

-        'for generally homogeneous data sets, should be approximately the '
-        'number of available CPU cores.')
+    super(ResnetArgParser, self).__init__(parents=[
+      official.utils.arg_parsers.LocationParser(data_dir=True, model_dir=True),


since each of line is kind of long, can we just import official.utils.arg_parsers as parsers, and use it has parsers.LocationParser?

ditto on above.

qlzh727 · 2018-03-13T16:34:15Z

official/utils/arg_parsers/parsers.py

+
+"""Collection of parsers which are shared among the official models.
+
+  The parsers in this module are intended to be used as parents to all arg


The indent in the beginning of this line is not necessary.

qlzh727 · 2018-03-13T17:12:31Z

official/utils/arg_parsers/parsers.py

+import argparse
+
+
+class LocationParser(argparse.ArgumentParser):


The naming of this class is bit confusing. This flag is trying to specify the file path, I think we should be more explicit here, eg DataPathParser or something else.

Moot point now, but I'll keep that in mind for the future.

qlzh727 · 2018-03-13T17:13:57Z

official/utils/arg_parsers/parsers.py

+    model_dir: Create a flag for specifying the model file directory.
+  """
+
+  def __init__(self, add_help=False, data_dir=False, model_dir=False):


param data_dir and model_dir give the impression that they should should be a path or string type, instead of boolean. Probably rename them into add_data_dir and add_model_dir.

Not sure if we should add "output_dir" here to specify the flag of the output directory for hooks logging?

I considered suffixing them with "_flag" (i.e. data_dir_flag=True, ...) but it made things look awkward and long. I am open to suggestions though.

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

official/utils/arg_parsers/parsers.py

+      )
+
+
+class DeviceParser(argparse.ArgumentParser):


This class only contains one flag, which is not necessary to wrap. Are you intend to add more flags into this parser?

qlzh727 · 2018-03-13T17:19:00Z

official/utils/arg_parsers/parsers.py

+      )
+
+
+class SupervisedParser(argparse.ArgumentParser):


Also a bit more explicit naming is better here, eg SupervisedLearningParser or CommonSupervisedLearningParser

Ditto on above.

qlzh727 · 2018-03-13T17:32:53Z

official/utils/arg_parsers/parsers.py

+      )
+
+
+class DummyParser(argparse.ArgumentParser):


DummyParse gives the impression that it does nothing, whereas it is actually doing something.

maybe DefaultParser?

Ditto on above.

qlzh727 · 2018-03-13T17:35:08Z

official/utils/arg_parsers/parsers.py

+  def __init__(self, add_help=False, data_dir=False, model_dir=False):
+    super(LocationParser, self).__init__(add_help=add_help)
+
+    if data_dir:


Since those are not random string and should be a file path, should we do some validation and make sure they exists?

I don't think the arg parser is the place for that, since a model may choose some behavior (i.e. prompt for a specific script to be run or download automatically) on a case by case basis.

qlzh727 · 2018-03-13T17:36:48Z

official/utils/arg_parsers/parsers_test.py

+
+    assert namespace.multi_gpu
+    assert namespace.use_synthetic_data
+


Do u also want to test the case that the flag value is specified while the arg parser is not turned on?

This turns out to not be practical. When parse_args fails, argparse fails hard by calling sys.exit(2) rather than raising an exception. While I certainly could invoke subprocess and assert an exit code of 2, this seems like too much machinery for a test that is just intended to show off parse_args().

karmel

Thanks.

karmel · 2018-03-13T23:30:19Z

official/utils/arg_parsers/parsers.py

+"""
+
+import argparse
+


Let's go ahead and make default=True for those as well. I would prefer to reduce the total amount of arg_parser code that users have to read through when looking at Resnet.

the new arg parsers. add parser unittests condense classes and make some style cleanups.

robieta requested review from k-w-w, karmel and nealwu as code owners March 12, 2018 20:51

googlebot added the cla: yes label Mar 12, 2018

karmel requested review from qlzh727 and yhliang2018 and removed request for nealwu March 13, 2018 16:21

karmel suggested changes Mar 13, 2018

View reviewed changes

qlzh727 reviewed Mar 13, 2018

View reviewed changes

robieta force-pushed the unified_arg_parser branch from c7eaae0 to 792b89f Compare March 13, 2018 20:29

karmel approved these changes Mar 13, 2018

View reviewed changes

Taylor Robie added 2 commits March 13, 2018 16:40

Create groups of arg parsers and convert the official resnet model to

5d8be90

the new arg parsers. add parser unittests condense classes and make some style cleanups.

make all arg parse defaults True.

4fe8648

robieta force-pushed the unified_arg_parser branch from 792b89f to 4fe8648 Compare March 13, 2018 23:41

robieta merged commit 086d914 into master Mar 13, 2018

robieta deleted the unified_arg_parser branch March 14, 2018 19:49

		@@ -0,0 +1,14 @@
		# Copyright 2017 The TensorFlow Authors. All Rights Reserved.



		class DummyParser(argparse.ArgumentParser):
		"""Default parser for specification of dummy/mocked behavior.


		"""Collection of parsers which are shared among the official models.

		The parsers in this module are intended to be used as parents to all arg

		import argparse


		class LocationParser(argparse.ArgumentParser):


		assert namespace.multi_gpu
		assert namespace.use_synthetic_data

Unified arg parser #3574

Unified arg parser #3574

Uh oh!

Conversation

robieta commented Mar 12, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment