Skip to content

Conversation

@chihuahua
Copy link
Member

Previously, #1132 had circumvented the use of tf.clip_by_value within the PR curves demo because it was buggy. However, that behavior seems to have been fixed. See tensorflow/tensorflow#18527.

Hence, we make the demo use tf.clip_by_value once again.

Previously, tensorflow#1132 had circumvented the use of `tf.clip_by_value` within the PR curves demo because it was buggy.

However, that behavior seems to have been fixed.
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 to revert back to using clip_by_value - and actually, I investigated a bit and I think the underlying problem is specific to the PR curve demo/test, versus a problem with clip_by_value per se.

The root of the issue is that tf.set_random_seed() only makes random values (as produced by functions like tf.distributions.Normal.sample) deterministic within the context of the exact same graph structure. If the graph structure changes, specifically if nodes are added/removed such that the node ID of the random op changes, then this directly changes the op-specific "deterministic" seed which gets used when the overall graph seed is fixed: https://github.com/tensorflow/tensorflow/blob/v1.8.0-rc1/tensorflow/python/framework/random_seed.py#L68

Our PR curves plugin integration test expects the values to be 100% deterministic, which means that graph structure changes that change node ID numbering of the sample() ops will break the test. I believe the changes in tf-nightly to clip_by_value() triggered this because clip_by_value() changed in terms of its internal op structure and that changed node ID numbers - but it's not the "fault" of clip_by_value().

In the short term, I figured out how to extract the op-specific seed values and will send a PR that uses fixed op-specific seeding so that adding/removing nodes won't break the test.

However, I don't think even with fixed graph and op seeds TF actually guarantees random op output values (I believe it guarantees only that they are deterministic for a given TF version, not that they won't change across versions). So the preferred fix would be removing the reliance of the test on randomness-based data generation, and instead do something like use a fake multiplexer populated with hardcoded fake PR curve tensor summaries.

@nfelt nfelt merged commit 4b25417 into tensorflow:master Apr 26, 2018
@chihuahua
Copy link
Member Author

Thanks for investigating! Running the test on some static data seems like a good idea then.

nfelt added a commit that referenced this pull request Apr 27, 2018
This addresses the root cause of the issue detected and originally addressed in #1132, per my description in #1150 (review).
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