Support experiment name and description in uploader #3277

bileschi · 2020-02-21T17:36:14Z

Motivation for features / changes
Adds the ability to list, upload, and modify experiments with names & descriptions.
Technical description of changes
Adds user intent & checking for
Screenshots of UI changes

bazel run //tensorboard -- dev list

Total: 17 experiment(s)
https://tensorboard.dev/experiment/F0EqjQjYTOCk4AdY5oskbQ/
        Name        this is demoable now.
        Description a *description* 💁👌🎍😍
        Created     2020-02-21 12:06:42 (23 minutes ago)
        Updated     2020-02-21 12:30:07 (6 seconds ago)
        Scalars     18807
        Runs        21
        Tags        7
https://tensorboard.dev/experiment/xDROKjLAS0eKh9Lfpdwamg
        Name        [No Name]
        Description [No Description]
...

* Detailed steps to verify changes work correctly (as executed by you)

bazel run //tensorboard -- dev upload --logdir /tmp/scalars_demo
bazel run //tensorboard -- dev upload --logdir /tmp/scalars_demo --name="my first name"
bazel run //tensorboard -- dev upload --logdir /tmp/scalars_demo --name="my first name" --description="**********************************************************************************************************************************************************************************************************************************************"
bazel run //tensorboard -- dev update-metadata --experiment_id=F0EqjQjYTOCk4AdY5oskbQ --description 'a description 💁👌🎍😍'


* Alternate designs / implementations considered
@googlers see API design doc

wchargin · 2020-02-21T17:42:07Z

tensorboard/uploader/uploader.py

+    """
+    logger.info("Modifying experiment %r", experiment_id)
+    if name is not None:
+        logger.info("Setting exp %r name to ", experiment_id, name)


Too many arguments for format string; you need another %r.

Done, thanks

wchargin · 2020-02-21T17:49:41Z

tensorboard/uploader/uploader.py

+    request = write_service_pb2.UpdateExperimentRequest(
+        experiment=experiment, experiment_mask=experiment_mask
+    )


Note that this deep-copies the protos (automatically, to avoid shared
mutable state). To avoid this, we typically populate fields on the
request directly:

request = write_service_pb2.UpdateExperimentRequest() request.experiment.experiment_id = experiment_id if name is not None: logger.info(...) request.experiment.name = name request.experiment_mask.name = True if description is not None: logger.info(...) request.experiment.description = description request.experiment_mask.description = True

(assuming that request.experiment_mask.SetInParent() does not change
the semantics; add that if it would).

Good to know re copy semantics. Easy to forget.

wchargin · 2020-02-21T17:53:20Z

tensorboard/uploader/uploader_main.py

+    if name and len(name) > _EXPERIMENT_NAME_MAX_CHARS:
+        raise ValueError(
+            "Experiment name is too long.  Limit is "
+            f"{_EXPERIMENT_NAME_MAX_CHARS} characters.\n"


This is a PY2AND3 module, but f-strings are a PY3-only feature, and
don’t provide enough value to change this whole target to PY3 (which is
also not yet possible due to Google-internal infrastructure; I’m
actively working on this). Please revert to normal format strings (of
either style), lest this break at sync time.

good catch, thanks.

wchargin · 2020-02-21T17:55:14Z

tensorboard/uploader/uploader_main.py

+        print("Modified experiment %s." % experiment_id)
+        if self.name is not None:
+            print(f"Set name to {repr(self.name)}")
+        if self.description is not None:
+            print(f"Set description to {repr(self.description)}")


UX: It’s fine to logging.info here for debugging, but it’s good form
for commands like this to print nothing on success (consider Unix
chmod/mv/sed, VCS hg reword/git amend, etc.). Using stdout for
“everything is OK” messages is a very Windows thing to do. :-)

FYI : the delete command above prints such a message. Should we remove it?

On second thought, I'm leaving them as logging.info, as you suggested.

True, delete does print. IMHO, this is a bit more justified because
permanently deleting an experiment is a more dangerous operation than
updating metadata, so the deletion confirmation is something that you
might want to show up in an audit log. For comparison, git rm prints
what it deleted, but git add does not print what it added.

If we want to remove the printing on delete in a separate PR, that’d
be fine with me. No strong opinion.

wchargin · 2020-02-21T17:56:15Z

tensorboard/uploader/uploader_main.py

@@ -417,7 +527,7 @@ def execute(self, server_info, channel):
                ("Tags", str(experiment.num_tags)),
            ]
            for (name, value) in data:
-                print("\t%s %s" % (name.ljust(10), value))
+                print("\t%s %s" % (name.ljust(11), value))


nit: Can we use 12 rather than 11 to keep aligned at an even column
boundary? Makes it a bit easier for folks to work with this text in a
normal editor.

Sure, done, but can you explain a little more why even column boundaries are better for normal editors?

Nothing huge; just that pressing <Tab> generally indents to the next
tab stop (if you have expandtab or equivalent set), and 12 is much
more likely to be that tab stop than 11 is. Hence “nit”. :-)

wchargin · 2020-02-21T17:57:10Z

tensorboard/uploader/uploader.py

+    )
+    try:
+        grpc_util.call_with_retries(writer_client.UpdateExperiment, request)
+    except grpc.RpcError as e:


Won’t we need to handle INVALID_ARGUMENT in the case where the server
raises a validation error? e.g., if we change the size limits on the
server side and people run update-metadata with an older client?

Done. I wasn't clear exactly how we want to handle this. I suppose we could pass along the details if they are populated?

Yep, passing along the details sounds good to me.

wchargin · 2020-02-21T18:00:55Z

tensorboard/uploader/uploader_test.py

+        eid_name = uploader_name.create_experiment()
+        self.assertEqual(eid_name, "123")
+        eid_description = uploader_description.create_experiment()
+        self.assertEqual(eid_description, "123")
+        eid_both = uploader_both.create_experiment()
+        self.assertEqual(eid_both, "123")


It would be nice to test that these actually send the proper requests.

Done. Broke this into 3 tests.

wchargin · 2020-02-21T18:03:05Z

tensorboard/uploader/uploader.py

+      ExperimentNotFoundError: If no such experiment exists.
+      PermissionDeniedError: If the user is not authorized to modify this
+        experiment.
+      RuntimeError: On unexpected failure.


Is this superfluous? The method doesn’t appear to raise RuntimeError
directly, and “we might raise an arbitrary exception due to a
programming error” isn’t really part of the public API (cf. style guide
on “Raises”).

Done. This was cloned to be consistent with delete_experiment below

Updated. This was here by copying from above.

wchargin · 2020-02-21T18:05:01Z

tensorboard/uploader/uploader_test.py

@@ -75,6 +75,25 @@ def test_create_experiment(self):
        eid = uploader.create_experiment()
        self.assertEqual(eid, "123")

+    def test_create_experiment_with_name_and_description(self):


Please add tests for update_metadata, including requests sent and
failure handling.

See UpdateExperimentMetadataTest below. Is that sufficient?

wchargin

Approved modulo fixing the error handling (.details()); thanks!

wchargin · 2020-02-21T22:58:54Z

tensorboard/uploader/uploader.py

+    )
+    try:
+        grpc_util.call_with_retries(writer_client.UpdateExperiment, request)
+    except grpc.RpcError as e:


Yep, passing along the details sounds good to me.

wchargin · 2020-02-21T23:12:07Z

tensorboard/uploader/uploader.py

+            details = ""
+            if hasattr(e, "details"):
+                details = e.details
+            raise InvalidArgumentError(details)


A few issues here:

On gRPC errors, code and details are callables, not the raw
values (note e.code() above). Thus, the actual exception message
is something like

<function grpc_error.<locals>.<lambda> at 0x7f18ea696f80>

which is not what was wanted. Unfortunately, this case is not
tested, so the failure is not caught.

Why the conditional? I was under the impression that details()
always exists on an RpcError value. If we do need the conditional,
it should just be getattr(e, "details", lambda: "")(). The
hasattr builtin is rarely what you want.

done. Added test for invalidArgument condition.

wchargin · 2020-02-21T23:12:46Z

tensorboard/uploader/uploader.py

+class InvalidArgumentError(RuntimeError):
+    pass


OK: We do have tensorboard.errors.InvalidArgumentError already, but
it’s reasonable to want to keep these local, and it’s not a problem to
change later if we change our minds.

tensorboard/uploader/uploader_main.py

wchargin · 2020-02-21T23:22:08Z

tensorboard/uploader/uploader_main.py

+        if flags.experiment_id:
+            if flags.name is not None or flags.description is not None:
+                return _UpdateMetadataIntent(
+                    flags.experiment_id,
+                    name=flags.name,
+                    description=flags.description,
+                )
+            else:
+                raise base_plugin.FlagsError(
+                    "Must specify either `--name` or `--description`."
+                )
+        else:
+            raise base_plugin.FlagsError(
+                "Must specify experiment to modify via `--experiment_id`."
+            )


nit: Can we use early returns here to avoid pushing the code more and
more to the right?

if not flags.experiment_id: raise base_plugin.FlagsError( "Must specify experiment to modify via `--experiment_id`." ) if flags.name is None and flags.description is None: raise base_plugin.FlagsError( "Must specify either `--name` or `--description`." ) return _UpdateMetadataIntent( flags.experiment_id, name=flags.name, description=flags.description, )

wchargin · 2020-02-21T23:36:01Z

tensorboard/uploader/uploader_test.py

+        mock_client = _create_mock_client()
+        new_description = """
+        **description**"
+        may have "strange" unicode chars 🌴 \/<>


The \/ is a syntax warning in Python 3.8+ and will be a syntax error
later (invalid escape sequence). Use \\/ or raw string literal?

wchargin · 2020-02-21T23:37:13Z

tensorboard/uploader/uploader.py

+    if name is not None:
+        logger.info("Setting exp %r name to %r", experiment_id, name)
+        request.experiment.name = name
+        request.experiment_mask.name = True


Not exactly in this PR, but it’s confusing that this field is not called
update_mask (per AIP 134); if it’s possible to change this before
stable launch, that would be great.

Not doing this in this PR. Let's discuss elsewhere if we can fit this in before stable launch.

wchargin · 2020-02-24T18:21:52Z

I’ve changed the title of this pull request to match our conventions
(imperative without trailing punctuation, which is standard for Git).

Friendly optional suggestion: You may find it convenient to enable your
editor to automatically format Python files with Black on save (with an
exception for google3). Then you just never have to think about it. :-)

* Adds name and description to the output of "list" subcommand * Adds name and description as upload options to the uploader * add subcommand for update-metadata to change name and description * black * adds additional testing. Fixes a logging error * Adds additional testing. Changes print output to log.info * black * stray keystroke * black * Adds test for INVALID_ARGUMENT. Addresses more reviewer critiques * black & stray edit * evenblacker

bileschi added 3 commits February 21, 2020 09:56

Adds name and description to the output of "list" subcommand

45ea650

Adds name and description as upload options to the uploader

cd79b29

add subcommand for update-metadata to change name and description

60cef9a

googlebot added the cla: yes label Feb 21, 2020

black

4aeed3c

wchargin self-requested a review February 21, 2020 17:44

adds additional testing. Fixes a logging error

1e69b93

wchargin reviewed Feb 21, 2020

View reviewed changes

bileschi added 4 commits February 21, 2020 16:14

Adds additional testing. Changes print output to log.info

a1e0ef8

black

68b82e4

stray keystroke

41fbac3

black

be7e7d6

bileschi requested a review from wchargin February 21, 2020 22:26

wchargin approved these changes Feb 21, 2020

View reviewed changes

bileschi added 3 commits February 24, 2020 12:03

Adds test for INVALID_ARGUMENT. Addresses more reviewer critiques

47762aa

black & stray edit

0d863a8

evenblacker

16f46d7

wchargin changed the title ~~Experiment name and description in uploader.~~ Support experiment name and description in uploader Feb 24, 2020

bileschi merged commit 2dee1bb into master Feb 24, 2020

stephanwlee deleted the exp_name_change branch February 24, 2020 19:42

bileschi mentioned this pull request Mar 3, 2020

TensorBoard 2.1.1 #3309

Merged

Support experiment name and description in uploader #3277

Support experiment name and description in uploader #3277

Conversation

bileschi commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin commented Feb 24, 2020