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: refactor to isolate TF dependencies #2163

Merged
merged 4 commits into from
Apr 30, 2019

Conversation

wchargin
Copy link
Contributor

Summary:
This sequence of commits turns the current api module into a wrapper
around two modules, callback (which depends on TensorFlow and only
provides the Keras callback) and summary_v2 (which does not, and
provides everything else). Thus, it is now possible in principle to
write hparams summaries without a TensorFlow dependency, though in
practice summary_v2 is currently implemented in terms of summary,
which depends on the TensorFlow copy of the Summary proto.

This is an internal refactor; the demo code is unchanged.

Reorganization to match the structure described here:
#2139 (comment)

Test Plan:
Existing unit tests retained, up to refactoring. The demo still works.

wchargin-branch: hparams-api-refactor

@wchargin
Copy link
Contributor Author

It is much easier to review these commits individually.

@wchargin wchargin requested a review from nfelt April 27, 2019 00:12
@wchargin wchargin marked this pull request as ready for review April 27, 2019 00:12
tensorboard/plugins/hparams/api.py Show resolved Hide resolved
@wchargin wchargin changed the base branch from wchargin-hparams-experiment-writing to master April 30, 2019 02:30
Summary:
Fixes an unintentional omission from #2130: `api.py` (not just its test)
actually does currently depend on TensorFlow.

Part of a reorganization to match the structure described here:
<#2139 (comment)>

wchargin-branch: hparams-api-refactor
Summary:
We’ll shortly reinstate `api` as a wrapper over `summary_v2` (in a
separate commit to help Git track the moves).

Part of a reorganization to match the structure described here:
<#2139 (comment)>

wchargin-branch: hparams-api-refactor
Summary:
We’ll shortly extract the Keras callback out of `summary_v2` such that
`summary_v2` does not need to depend on TensorFlow. It will continue to
be exposed from `api`.

Part of a reorganization to match the structure described here:
<#2139 (comment)>

wchargin-branch: hparams-api-refactor
Summary:
The `summary_v2` module no longer has any fundamental dependency on
TensorFlow. Its implementation still depends on `summary`, which does
directly reference the TensorFlow proto copies.

Part of a reorganization to match the structure described here:
<#2139 (comment)>

wchargin-branch: hparams-api-refactor
@wchargin wchargin force-pushed the wchargin-hparams-api-refactor branch from 4d46798 to efe8788 Compare April 30, 2019 02:49
@wchargin wchargin merged commit bee91f6 into master Apr 30, 2019
@wchargin wchargin deleted the wchargin-hparams-api-refactor branch April 30, 2019 05:17
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