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

histogram: set v3 as default #5415

Merged
merged 4 commits into from
Nov 12, 2021
Merged

histogram: set v3 as default #5415

merged 4 commits into from
Nov 12, 2021

Conversation

yatbear
Copy link
Member

@yatbear yatbear commented Nov 11, 2021

Set V3 implementation as default, dynamic shapes will be eliminated.

Ran internally: cl/408885623

#histogram #tpu

@google-cla google-cla bot added the cla: yes label Nov 11, 2021
@yatbear yatbear changed the title histogram: set V3 as default histogram: set v3 as default Nov 11, 2021
@yatbear yatbear requested a review from nfelt November 11, 2021 16:50
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.

Actually, if you don't mind, I'd recommend that in this PR we just do only the cutover - i.e. let's not remove the v2 codepath yet. Just in case there are issues and we have to roll back, that makes the rollback as minimal as possible and avoids extra churn in the code.

For example, I think it would be enough to just do the following:

  • in summary_v2.py
    • rename histogram to histogram_v2
    • add histogram = histogram_v3 at the bottom
  • in summary_test.py
    • update SummaryV2OpTest.call_histogram_op to invoke histogram_v2 instead of just histogram

@@ -16,13 +16,20 @@

A histogram summary stores a list of buckets. Each bucket is encoded as
a triple `[left_edge, right_edge, count]`. Thus, a full histogram is
encoded as a tensor of dimension `[k, 3]`.
encoded as a tensor of dimension `[k, 3]`, where the first `k - 1` buckets
Copy link
Contributor

Choose a reason for hiding this comment

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

This module docstring is all referring to the legacy "tb.summary" APIs (for TensorFlow 1.x) that were defined in this file, which we aren't changing, so I think it's ok to just leave this docstring as-is (and only have the new format documented in summary_v2.py's docstring). At some point we should rearrange and clean this up so that's more obvious but probably best to do that for all the summary APIs vs piecemeal.

Copy link
Member Author

@yatbear yatbear Nov 11, 2021

Choose a reason for hiding this comment

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

Done. Thanks for the suggestion!

@yatbear yatbear requested a review from nfelt November 11, 2021 21:16
histogram = summary_v2.histogram
histogram_pb = summary_v2.histogram_pb

# Export V3 versions.
histogram_v3 = summary_v2.histogram_v3
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this one line for now, so that anyone who was testing this out internally won't be broken by this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@yatbear yatbear merged commit 90f38da into tensorflow:master Nov 12, 2021
@yatbear yatbear deleted the v3_default branch November 12, 2021 19:42
yatbear added a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
* set histogram v3 as default

* remove unused import

* keep the v2 version for now

* keep the v3 export to avoid breaking internal tests
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
* set histogram v3 as default

* remove unused import

* keep the v2 version for now

* keep the v3 export to avoid breaking internal tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants