Skip to content

Conversation

@bileschi
Copy link
Collaborator

  • Motivation for features / changes
    Support for experiment name & description in TensorBoard.dev

  • Technical description of changes
    Extends proto service adding:
    UpdateExperimentMetadata
    OSS product users should see no chnage.

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

$ bazel test //tensorboard/... --test_tag_filters=-webtest --build_tests_only

(this is a continuation of a previous messed up PR #3227 )

@bileschi
Copy link
Collaborator Author

Investigating the failure of //tensorboard/pip_package:test_pip_package in Travis

@bileschi
Copy link
Collaborator Author

Updated to remove duplicate definition of Experiment. Moved single source of truth to within export_service.proto


import "google/protobuf/timestamp.proto";

// Metadata about an experiment.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good form to indicate (once we support updating some of this metadata) which fields are output-only and cannot be directly set by the client. (In this case, all of them except name and description.)

See e.g. https://aip.dev/203#output-only (I don't think we necessarily need to use the programmatic specifier, but having the documentation in a comment is still valuable)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, tricky because the metadata is used in several different locations for different purposes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nevertheless, this file has been removed.

@wchargin wchargin removed their request for review February 11, 2020 20:13
@wchargin
Copy link
Contributor

Delegating to others while I’m out; see my comments on the previous
#3227 and the internal http://cl/293665881 for my perspective.

@caisq
Copy link
Contributor

caisq commented Feb 11, 2020

cc myself

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

LGTM modulo the one comment left.

@bileschi bileschi merged commit 79b4a57 into master Feb 12, 2020
@bileschi bileschi deleted the exp_name_proto_2 branch February 12, 2020 16:04
bileschi added a commit to bileschi/tensorboard that referenced this pull request Mar 3, 2020
)

Proto changes to support experiment name & description.
@bileschi bileschi mentioned this pull request Mar 3, 2020
nfelt pushed a commit that referenced this pull request Mar 4, 2020
Proto changes to support experiment name & description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants