Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uploader: add options --dry_run and --one_shot #3707

Merged
merged 13 commits into from
Jun 5, 2020

Conversation

caisq
Copy link
Contributor

@caisq caisq commented Jun 5, 2020

  • Motivation for features / changes
    • Improve the usability of the tensorboard.dev uploader
  • Technical description of changes
    • Add --dry_run flag, which when enabled, causes the uploader to only read data
      from the logdir and display the statistics (if --verbose is the default 1) and upload
      no data to the server.
      • This is implemented through a dry-run stub: DryRunTensorBoardWriterStub.
    • Add --one_shot flag, which when enabled, causes the uploader to exit immediately
      after all existing data in the logdir are uploaded.
      • This mode prints a warning message if the logdir doesn't contain any uploadable
        data.
  • Screenshots
    • Dry run, one shot, non-empty logdir: image
    • Dry run, one shot, empty logdir: image

@caisq caisq marked this pull request as ready for review June 5, 2020 03:14
@caisq caisq requested a review from davidsoergel June 5, 2020 03:15
@caisq
Copy link
Contributor Author

caisq commented Jun 5, 2020

cc @GalOshri

@@ -103,6 +104,10 @@ def __init__(
verbosity: Level of verbosity, an integer. Supported value:
0 - No upload statistics is printed.
1 - Print upload statistics while uploading data (default).
one_shot: Once uploading starts, upload only the existing data in and
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous "and"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"--one_shot",
action="store_true",
help="Upload only the existing data in the logdir and then exit "
"immediately, instead of keep listening for new data in the logdir.",
Copy link
Member

Choose a reason for hiding this comment

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

...instead of continuing to listen...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -185,6 +185,45 @@ def testHasNewDataSinceLastSummarizeReturnsTrueAfterNewTensors(self):
stats.add_blob(blob_bytes=2000, is_skipped=True)
self.assertEqual(stats.has_new_data_since_last_summarize(), True)

def testHasDataInitiallyReturnsFlase(self):
Copy link
Member

Choose a reason for hiding this comment

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

typo: False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

)
self.assertEqual(stats.has_data(), True)

def testHasDataReturnsTrueWithskippedTensors(self):
Copy link
Member

Choose a reason for hiding this comment

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

typo: capitalize Skipped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

def CreateExperiment(self, request, **kwargs):
"""Create a new experiment and remember it has been created."""
del request, kwargs # Unused.
return write_service_pb2.CreateExperimentResponse()
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little surprised that all the responses can be empty here (except the BlobSequence case, I see). OK, assuming you've tested a dry run with scalar, tensor, and blob data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Manually tested such logdirs with --dry_run.

@caisq caisq merged commit c9f29e1 into tensorflow:master Jun 5, 2020
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.

3 participants