Skip to content

Conversation

@wchargin
Copy link
Contributor

@wchargin wchargin commented Aug 8, 2019

Summary:
These tests were disabled for two reasons: they used tf.placeholder,
and they used tf.contrib.summary for DB mode tests. This patch revises
the implementation of the non–DB mode tests to avoid using placeholders,
and enables those tests in all versions of TensorFlow. The DB mode tests
still cannot be run in TF 2.x because create_db_writer is not exported
under any public APIs.

Test Plan:
Tests pass in current tf-nightly and tf-nightly-2.0-preview.

wchargin-branch: scalars-plugin-tests-tf2x

Summary:
These tests were disabled for two reasons: they used `tf.placeholder`,
and they used `tf.contrib.summary` for DB mode tests. This patch revises
the implementation of the non–DB mode tests to avoid using placeholders,
and enables those tests in all versions of TensorFlow. The DB mode tests
still cannot be run in TF 2.x because `create_db_writer` is not exported
under any public APIs.

Test Plan:
Tests pass in current `tf-nightly` and `tf-nightly-2.0-preview`.

wchargin-branch: scalars-plugin-tests-tf2x
@wchargin wchargin requested a review from nfelt August 8, 2019 02:30
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.

Thanks for the cleanup!

feed_dict = {placeholder: [1 + step, 2 + step, 3 + step]}
s = sess.run(summ, feed_dict=feed_dict)
writer.add_summary(s, global_step=step)
placeholder = [1 + step, 2 + step, 3 + step]
Copy link
Contributor

Choose a reason for hiding this comment

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

placeholder doesn't really make sense as a name for this since it is no longer an actual placeholder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to data.

@nfelt
Copy link
Contributor

nfelt commented Aug 9, 2019

Also, this fixes most of the scalars_plugin_test.py entry for #1705 so we should probably update that when this goes in (we can perhaps break out a separate line for the tests that use the db_writer as a straggler cleanup item).

wchargin-source: 21849b2fe9386a20a8f884b8e103c5e18e17ce3a
wchargin-branch: scalars-plugin-tests-tf2x
@wchargin wchargin merged commit afbdf87 into master Aug 9, 2019
@wchargin wchargin deleted the wchargin-scalars-plugin-tests-tf2x branch August 14, 2019 03:42
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