program: add subcommand CLI support #2793

wchargin · 2019-10-17T22:08:53Z

Summary:
This commit adds a subcommand interface to support invocations in the
manner of apt-get install or git commit. All existing invocations
remain valid, and are now also available under the tensorboard serve
subcommand. Other subcommands may be injected programmatically. For
instance, we could migrate the existing tensorboard --inspect to
tensorboard inspect, which makes more sense semantically (as it does
something quite different from starting a web server).

To support both tensorboard --flags and tensorboard serve --flags,
we require some hacks first to get off the ground at all in Python 2,
and then to have sane error messages without massive flag duplication in
all Python versions. Even recent Python versions don’t have a notion of
a default subcommand, so we need to manually inspect argv to
determine what kind of argument parser to construct.

Test Plan:
Unit tests added, and the smoke test for the dynamic plugin (plus the
Pip package test script) serve as integration tests for existing
behavior. It’s important to test this in both Python 2 and Python 3. For
manual testing:

Check that the output of tensorboard --help still includes all the
normal flags, and now has a list of subcommands (one item long).
Check that the output of tensorboard serve --help is just like the
output of tensorboard --help, but without the list of subcommands.
Patch a demo subcommand into main.py, and check that the output
of tensorboard demo --help is just that subcommand’s help (without
flags to serve) and that tensorboard --help shows the demo
subcommand.

Finally, note that changing the implementation of the monkey patch to
just yield; return causes both argparse_util_test and program_test
to fail on Python 2 (but not Python 3).

wchargin-branch: subcommands

Summary: This commit adds a subcommand interface to support invocations in the manner of `apt-get install` or `git commit`. All existing invocations remain valid, and are now also available under the `tensorboard serve` subcommand. Other subcommands may be injected programmatically. For instance, we could migrate the existing `tensorboard --inspect` to `tensorboard inspect`, which makes more sense semantically (as it does something quite different from starting a web server). To support both `tensorboard --flags` and `tensorboard serve --flags`, we require some hacks first to get off the ground at all in Python 2, and then to have sane error messages without massive flag duplication in all Python versions. Even recent Python versions don’t have a notion of a _default_ subcommand, so we need to manually inspect `argv` to determine what kind of argument parser to construct. Test Plan: Unit tests added, and the smoke test for the dynamic plugin (plus the Pip package test script) serve as integration tests for existing behavior. It’s important to test this in both Python 2 and Python 3. For manual testing: - Check that the output of `tensorboard --help` still includes all the normal flags, and now has a list of subcommands (one item long). - Check that the output of `tensorboard serve --help` is just like the output of `tensorboard --help`, but without the list of subcommands. - Patch a `demo` subcommand into `main.py`, and check that the output of `tensorboard demo --help` is just that subcommand’s help (without flags to `serve`) and that `tensorboard --help` shows the `demo` subcommand. wchargin-branch: subcommands

wchargin · 2019-10-18T05:11:25Z

All existing invocations remain valid, and are now also available
under the tensorboard serve subcommand.

Another idea: we could call this start, by analogy with npm start
and friends. Open to input.

nfelt

LGTM! Let the subcommand proliferation begin :D

nfelt · 2019-10-18T20:52:51Z

tensorboard/program.py

-    flags = parser.parse_args(argv[1:])  # Strip binary name from argv.
+
+    argparse_monkey_patch = _ArgparseMonkeyPatch()
+    argparse_monkey_patch.install()


Maybe just make this a context manager if we only need to install/uninstall in a context-manager-friendly context?

Good idea; thanks! Done.

nfelt · 2019-10-18T20:54:56Z

tensorboard/program.py

+      efi.inspect(flags.logdir, event_file, flags.tag)
      return 0
-    if self.flags.version_tb:
+    if flags.version_tb:


Can you add a TODO(#2801) here? The version flag really should be installed only on the base parser, not as a subflag of serve, but that requires moving it out of the plugin-centric flag definition system.

nfelt · 2019-10-18T21:03:00Z

tensorboard/program.py

    logging.getLogger('werkzeug').setLevel(logging.NOTSET)


+class _ArgparseMonkeyPatch(object):


Optional, but maybe nicer to put this in an argparse_util.py or something since it's otherwise not really relevant to program.py.

nfelt · 2019-10-18T21:04:59Z

tensorboard/program.py

+class _ArgparseMonkeyPatch(object):
+  """Make Python 2.7 behave like Python 3 w.r.t. default subcommands.
+
+  The behavior of argparse was changed in Python 3.3. When a parser


Might as well add a reference to https://bugs.python.org/issue16308 and python/cpython@f97c59a for the curious?

Meant to do this, yes; thanks!

wchargin-branch: subcommands wchargin-source: 1a3423e24da6b619baf06c26dbd3fbfdd473970a

nfelt · 2019-10-19T01:22:05Z

tensorboard/util/argparse_util.py

+      return real_error(*args, **kwargs)
+
+  argparse.ArgumentParser.error = error
+  yield


We want a try-finally around the yield, right? As shown in https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager

Agh, yes, of course. For some reason I thought that contextlib handled
that for us, but of course it can’t, because _handling_errors and
friends need to be able to respond to errors. Thanks; fixed.

nfelt · 2019-10-19T01:22:43Z

tensorboard/util/argparse_util_test.py

+from tensorboard.util import argparse_util
+
+
+class AllowMissingSubcommandTest(tb_test.TestCase):


Awesome, thanks for adding these tests too!

wchargin-branch: subcommands wchargin-source: b9448d785b8bfdcf458df60ed0ff0da991270f03

Summary: This commit adds a subcommand interface to support invocations in the manner of `apt-get install` or `git commit`. All existing invocations remain valid, and are now also available under the `tensorboard serve` subcommand. Other subcommands may be injected programmatically. For instance, we could migrate the existing `tensorboard --inspect` to `tensorboard inspect`, which makes more sense semantically (as it does something quite different from starting a web server). To support both `tensorboard --flags` and `tensorboard serve --flags`, we require some hacks first to get off the ground at all in Python 2, and then to have sane error messages without massive flag duplication in all Python versions. Even recent Python versions don’t have a notion of a _default_ subcommand, so we need to manually inspect `argv` to determine what kind of argument parser to construct. Test Plan: Unit tests added, and the smoke test for the dynamic plugin (plus the Pip package test script) serve as integration tests for existing behavior. It’s important to test this in both Python 2 and Python 3. For manual testing: - Check that the output of `tensorboard --help` still includes all the normal flags, and now has a list of subcommands (one item long). - Check that the output of `tensorboard serve --help` is just like the output of `tensorboard --help`, but without the list of subcommands. - Patch a `demo` subcommand into `main.py`, and check that the output of `tensorboard demo --help` is just that subcommand’s help (without flags to `serve`) and that `tensorboard --help` shows the `demo` subcommand. Finally, note that changing the implementation of the monkey patch to just `yield; return` causes both `argparse_util_test` and `program_test` to fail on Python 2 (but not Python 3). wchargin-branch: subcommands

Summary: In #2793, we added a helper to ensure that you can run `tensorboard` either with or without an explicit subcommand. This works by default in Python 3; the helper was only needed for Python 2. Thus, we remove it. Part of #4488. Test Plan: The test plan from #2793 still passes. Instead of patching in a `demo` subcommand, you can now use `tensorboard dev list [--json]`. wchargin-branch: py3-no-argparseutil wchargin-source: 0f5517b1b09e4fae5158096d55cf8b6247b2e2e2

Summary: In #2793, we added a helper to ensure that you can run `tensorboard` either with or without an explicit subcommand. This works by default in Python 3; the helper was only needed for Python 2. Thus, we remove it. Part of #4488. Test Plan: The test plan from #2793 still passes. Instead of patching in a `demo` subcommand, you can now use `tensorboard dev list [--json]`. wchargin-branch: py3-no-argparseutil

wchargin added the core:backend label Oct 17, 2019

wchargin requested a review from nfelt October 17, 2019 22:08

googlebot added the cla: yes label Oct 17, 2019

nfelt approved these changes Oct 18, 2019

View reviewed changes

[update patch]

e0986b7

wchargin-branch: subcommands wchargin-source: 1a3423e24da6b619baf06c26dbd3fbfdd473970a

nfelt approved these changes Oct 19, 2019

View reviewed changes

[update patch]

e511faf

wchargin-branch: subcommands wchargin-source: b9448d785b8bfdcf458df60ed0ff0da991270f03

wchargin merged commit 20e80a2 into master Oct 19, 2019

wchargin deleted the wchargin-subcommands branch October 19, 2019 06:55

wchargin mentioned this pull request Oct 29, 2019

TensorBoard 2.0.1 #2859

Merged

wchargin mentioned this pull request Dec 17, 2020

py3: remove argparse_util helper #4493

Merged

		logging.getLogger('werkzeug').setLevel(logging.NOTSET)


		class _ArgparseMonkeyPatch(object):

		from tensorboard.util import argparse_util


		class AllowMissingSubcommandTest(tb_test.TestCase):

program: add subcommand CLI support #2793

program: add subcommand CLI support #2793

Uh oh!

Conversation

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

Uh oh!

Uh oh!

wchargin commented Oct 18, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wchargin commented Oct 17, 2019 •

edited

Loading