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

hparams: account for proto nondeterminism in tests #2235

Merged
merged 2 commits into from
May 15, 2019

Conversation

wchargin
Copy link
Contributor

Summary:
This test seems to be fine in open-source land, but failed 18/32 times
within Google, because the wire format order for maps is undefined:
https://developers.google.com/protocol-buffers/docs/proto3#maps

Test Plan:
Within google3, cherry-pick this patch onto http://cl/247655537, then
run :summary_v2_notf_test with --runs_per_test=128 to check for
flakiness.

wchargin-branch: hparams-test-deflake

Summary:
This test seems to be fine in open-source land, but failed 18/32 times
within Google, because the wire format order for maps is undefined:
<https://developers.google.com/protocol-buffers/docs/proto3#maps>

Test Plan:
Within google3, cherry-pick this patch onto <http://cl/247655537>, then
run `:summary_v2_notf_test` with `--runs_per_test=128` to check for
flakiness.

wchargin-branch: hparams-test-deflake
wchargin-branch: hparams-test-deflake
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

Honest question: does self.assertProtoEquals solve the issue?

@wchargin
Copy link
Contributor Author

Nope. The protos (summary_1 and summary_2) are genuinely different,
but only insofar as the ....content fields are bytestrings that
contain different wire format representations of the same logical
object. Here’s the error (from my comment on the linked CL):

AssertionError: value {
  tag: "_hparams_/session_start_info"
  metadata {
    plugin_data {
      plugin_name: "hparams"
      content: "\032|\n\023\n\toptimizer\022\006\032\004adam\n\032\n\rlearning_rate\022\t\021{\024\256G\341z\224?\"@582419390b277d40968d8485643f1f8b22bb547061539ecf176b28630acfe4be)\315\314\314\314\314\334^@"
    }
  }
}
 != value {
  tag: "_hparams_/session_start_info"
  metadata {
    plugin_data {
      plugin_name: "hparams"
      content: "\032|\n\032\n\rlearning_rate\022\t\021{\024\256G\341z\224?\n\023\n\toptimizer\022\006\032\004adam\"@582419390b277d40968d8485643f1f8b22bb547061539ecf176b28630acfe4be)\315\314\314\314\314\334^@"
    }
  }
}

Using assertProtoEquals would give a nicer error message, but it
wouldn’t consider two distinct bytestrings as equal.

Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

Ah of course the content is serialized proto bytestring. I missed that bit. Thanks

@wchargin wchargin merged commit 97d048a into master May 15, 2019
@wchargin wchargin deleted the wchargin-hparams-test-deflake branch May 15, 2019 23:05
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