Skip to content

Conversation

orionr
Copy link
Contributor

@orionr orionr commented Feb 12, 2019

Continuation of #1663

This adds plug and TensorFlow compat tests both locally running and in Travis CI.

cc @nfelt, @lanpa

All the tests can be run with

bazel test //tensorboard/compat/proto:proto_test --test_output=errors
bazel test //tensorboard/compat/tensorflow_stub:gfile_test --test_output=errors
bazel test //tensorboard/compat/tensorflow_stub:gfile_s3_test --test_output=errors
bazel test //tensorboard/plugins/audio:audio_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/custom_scalar:custom_scalars_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/distribution:distributions_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/graph:graphs_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/histogram:histograms_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/image:images_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/pr_curve:pr_curves_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/projector:projector_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/scalar:scalars_plugin_notf_test --test_output=errors
bazel test //tensorboard/plugins/text:text_plugin_notf_test --test_output=errors

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

If I understand correctly, only the second commit in this pull request
(“Add plugin and compat tests for tensorboard-notf target”) is intended
to be reviewed, yes?

@orionr orionr force-pushed the add-plugin-and-compat-tests branch from 2cb674b to bcf4c29 Compare February 12, 2019 20:56
@orionr
Copy link
Contributor Author

orionr commented Feb 12, 2019

@wchargin that's correct. I can remove the other if we don't mind test failures right now. What would you prefer? Thank you.

@wchargin
Copy link
Contributor

Keeping the commits as-is is fine; once #1663 merges, you can update
this PR.

I don’t have too much context here, but the second commit seems pretty
unobjectionable, so I think that this is basically blocked on #1663.

@orionr orionr force-pushed the add-plugin-and-compat-tests branch 2 times, most recently from 88ff079 to 2f64e92 Compare February 22, 2019 19:47
@orionr
Copy link
Contributor Author

orionr commented Feb 22, 2019

This has been rebased on top of master and should be ready for review. Thanks, @nfelt and @wchargin!

@wchargin wchargin self-requested a review February 22, 2019 23:37
@orionr orionr force-pushed the add-plugin-and-compat-tests branch from 2f64e92 to 8b543f7 Compare February 25, 2019 18:31
@orionr
Copy link
Contributor Author

orionr commented Feb 25, 2019

Rebased.

@orionr
Copy link
Contributor Author

orionr commented Feb 25, 2019

@wchargin any interest in reviewing and landing this? @nfelt also feel free to comment if you have concerns.

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.

This is not the right long-term solution, but for now it improves test coverage, so we can merge once the projector tests are cleaned up per comment below.

Can you also merge in the latest changes from master?

@orionr orionr force-pushed the add-plugin-and-compat-tests branch from 8b543f7 to 1d2dacf Compare March 14, 2019 14:54
@orionr
Copy link
Contributor Author

orionr commented Mar 14, 2019

Need to upgrade bazel to test locally, but just rebased and pushed changes to match the review. Let me know your thoughts and thanks.

@orionr orionr force-pushed the add-plugin-and-compat-tests branch from bbba0a5 to ed22a68 Compare March 14, 2019 16:23
@orionr
Copy link
Contributor Author

orionr commented Mar 19, 2019

This should be ready to merge @nfelt and @wchargin. Thanks.

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 updates!

@nfelt nfelt merged commit 09d6c47 into tensorflow:master Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants