Proto changes to support experiment name & description. #3227
Conversation
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
bazel test tensorboard:all in a virtual env
Note that this only runs top-level tests; to easily run all non-frontend
tests, you can invoke:
bazel test //tensorboard/... --test_tag_filters=-webtest --build_tests_only
| } | ||
|
|
||
| // Request to change the name of one experiment. | ||
| message SetExperimentNameRequest{ |
There was a problem hiding this comment.
Consider combining these into one RPC, with a fieldmask? Like:
message UpdateExperimentMetadataRequest {
Experiment updates = 1;
ExperimentMask update_mask = 2;
}
message ExperimentMask {
reserved 1 to 6;
reserved "experiment_id", "create_time", "update_time", "num_scalars",
"num_runs", "num_tags";
bool name = 7;
bool description = 8;
}(edited to fix ExperimentMetadataMask to ExperimentMask—thanks,
@davidsoergel)
It seems common that the client will want to set both the name and
description at once, and doing so via two RPC roundtrips and two
database transactions (can’t parallelize because of contention) seems
wasteful.
Note that Google standard is to call the method UpdateFoo (not
SetFoo; this aligns with HTTP verbs), and to just pass the object as
both the request and the response.
|
|
||
| // Request to change the name of one experiment. | ||
| message SetExperimentNameRequest{ | ||
| // Permanent ID of this experiment. Should match the id in the request. |
There was a problem hiding this comment.
What does “should match the ID in the request” mean? This is the
request.
| } | ||
|
|
||
| // Request to change the name of one experiment. | ||
| message SetExperimentNameRequest{ |
There was a problem hiding this comment.
nit: Could you run clang-format on this file, please? (We don’t have a
hard lint check for it in OSS because we change these files
infrequently, but I can add one.)
| ("Name", experiment.name), | ||
| ("Description", experiment.description), |
There was a problem hiding this comment.
If you do make this change, please change the name.ljust(10) padding
below such that Description doesn’t abut into the next column.
| int64 num_tags = 6; | ||
| // User provided name of the experiment. | ||
| string name = 7; | ||
| // User provided desription of the experiment, in markdown source format. |
There was a problem hiding this comment.
sp.: s/desription/description/
Summary: We keep our protos formatted with `clang-format`, but hitherto have not enforced this, inviting regressions. This commit adds a CI lint check. Test Plan: Verified that this fails on CI before the `custom_scalars/layout.proto` fix, and passes afterward. wchargin-branch: ci-clang-format-proto
This unblocks the sync which failed because of the strict_deps violation. - tensorboard/webapp/webapp_data_source/tb_server_data_source_module.ts had extraneous import statement. - tb_http_client_testing.ts had missing import on BUILD.
| ExperimentMetadataMask update_mask = 2; | ||
| } | ||
|
|
||
| message ExperimentMetadataMask { |
There was a problem hiding this comment.
Naming nit: ExperimentMetadataMask does not have an obviously different meaning from ExperimentMask. How about ExperimentUpdateMask?
There was a problem hiding this comment.
Or UpdateExperimentMask, etc.
There was a problem hiding this comment.
Yes, thank you—it should be ExperimentMask. (Vestige of earlier draft
of comment.)
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
This change has build and test errors. Could you please fix the build,
run the tests, and re-request review once that’s done?
Although the werkzeug changelog did not say so, it seem to have removed certain symbols from `werkzeug`. We now use `werkzeug.wrappers.BaseResponse` instead. Context: https://werkzeug.palletsprojects.com/en/master/changes/#version-1-0-0
|
Cancelling this as it got cobbed up in a merge |
Okay—for future reference, it’s nice to try to keep these in the same It’s not quite as nice in the GitHub UI, but it’s still much nicer than (In this case, now that you’ve opened #3234, we might as well just use |
|
Thanks! Yeah, sorry I was spinning my wheels on getting the graph to only include the change of interest and so I decided to cut my losses and move the change manually. I'll try this more civilized fix next time. |
5 participants
Motivation for features / changes
Support for experiment name & description in TensorBoard.dev
Technical description of changes
Extends proto service adding:
SetExperimentName and SetExperimentDescription
Extends uploader to read those fields during export
Screenshots of UI changes
OSS product users should see no chnage.
Detailed steps to verify changes work correctly (as executed by you)
bazel test tensorboard:all in a virtual env