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

[CMake] Add TensorBoard dependencies to PIP package. #5844

Merged
merged 3 commits into from Dec 1, 2016

Conversation

Projects
None yet
9 participants
@vit-stepanovs
Contributor

vit-stepanovs commented Nov 25, 2016

Currently, TensorBoard external dependencies (JS scripts, css, images etc.) are not part of PIP package built by CMake. As a result, when you navigate to TensorBoard on Windows, it shows a blank screen. Added CMake scripts to download TensorBoard dependencies and make them part of PIP package.

@tensorflow-jenkins

This comment has been minimized.

Collaborator

tensorflow-jenkins commented Nov 25, 2016

Can one of the admins verify this patch?

@mention-bot

This comment has been minimized.

mention-bot commented Nov 25, 2016

@vit-stepanovs, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrry, @danmane and @guschmue to be potential reviewers.

@googlebot googlebot added the cla: yes label Nov 25, 2016

@gunan

This comment has been minimized.

Member

gunan commented Nov 25, 2016

Jenkins, test this please.

@vit-stepanovs

This comment has been minimized.

Contributor

vit-stepanovs commented Nov 25, 2016

Looks like build failed because multiple dependencies have the same downloadable archive name... Not sure why this passed for me locally. Will make a change to download each archive in a separate directory.

NORTHAMERICA\vistepan
@mrry

This comment has been minimized.

Contributor

mrry commented Nov 28, 2016

@tensorflow-jenkins test this please.

add_python_module("tensorflow/contrib/tensorboard")
add_python_module("tensorflow/contrib/tensorboard")
add_python_module("tensorflow/contrib/tensorboard/plugins")
add_python_module("tensorflow/contrib/tensorboard/plugins/projector")

This comment has been minimized.

@mrry

mrry Nov 28, 2016

Contributor

These lines (with the exception of the duplicated line 378) still seem important... without them I'd expect importing tf.contrib to fail. Are they covered by some other rule now?

This comment has been minimized.

@vit-stepanovs

vit-stepanovs Nov 28, 2016

Contributor

The other lines are also duplicates - the same lines can be found above at 358-360.

This comment has been minimized.

@mrry

mrry Nov 28, 2016

Contributor

Ack, thanks for catching that!

set(tensorboard_dependencies ${tensorboard_dependencies} ${_TB_NAME} PARENT_SCOPE)
endfunction()
include(external/tensorboard_deps.cmake)

This comment has been minimized.

@mrry

mrry Nov 28, 2016

Contributor

This file seems horrific - from a maintenance standpoint - but we should be able to generate it automatically (in the same manner as this file).

This comment has been minimized.

@vit-stepanovs

vit-stepanovs Nov 28, 2016

Contributor

I generated this file myself off WORKSPACE/bower.BUILD files, hence it is separate from tensorboard.cmake. I am happy to share my script if needed, but I think it may be better to modify the one script that updates WORKSPACE/bower.BUILD to do the same to this file (from a maintenance perspective, I guess it is better to have one script).

This comment has been minimized.

@jart

jart Nov 28, 2016

Member

I'm a TensorBoard developer. I've never used cmake before. I don't know how I would maintain this. What would you recommend for me?

This comment has been minimized.

@mrry

mrry Nov 28, 2016

Contributor

@jart We already produce a similar file for Bazel. We could modify the script that auto-generates bower.BUILD (and the suffix of WORKSPACE) so that it also produces the contents of external/tensorboard_deps.cmake. Unfortunately that script isn't in the git repository, so we can't easily do it in this PR.

This comment has been minimized.

@jart

jart Nov 29, 2016

Member

I'm concerned because the TensorBoard team changes these external dependency definitions about once a week. I want to make sure the additional burden on the team is as minimal as possible.

It would be nice if this information could be extracted from WORKSPACE and bower.BUILD without making any modifications to either of those files. The cmake build already surgically extracts similar information from the workspace.bzl file for Eigen.

Also is Jenkins going to be regression testing this change?

This comment has been minimized.

@vit-stepanovs

vit-stepanovs Nov 29, 2016

Contributor

I was under impression that WORKSPACE/bower.BUILD files are already automatically updated with newest TensorBoard dependencies by running some script (at least the comments in those files suggest so). So it might be possible to update that script to modify tensorboard_deps.cmake automatically too whenever WORKSPACE/bower.BUILD are updated. That way, isn't the burden minimal?

This comment has been minimized.

@jart

jart Nov 30, 2016

Member

That script is going away soon, because the input for that script is bower.json which is also going away. We're getting rid of Gulp and Bower entirely and rewriting the TensorBoard build from scratch in Bazel. Once that's done, the Bazel files are going to be the source of truth for these dependencies.

Therefore, if you are kind enough to write cmake code that is able to parse those bazel files to extract the definition, then it should be easy for me—as someone who knows next to nothing about cmake—to maintain what you did going forward.

This comment has been minimized.

@jart

jart Nov 30, 2016

Member

Also take into consideration that Bazel TensorFlow is officially coming to Windows. The TensorBoard team is committed to making the new TensorBoard Bazel build fully functional on Windows, so Windows users can participate in TensorBoard development if they choose.

This comment has been minimized.

@vit-stepanovs

vit-stepanovs Dec 1, 2016

Contributor

I have changed tensorboard.cmake to parse dependencies from Bazel files.

@mrry

mrry approved these changes Nov 28, 2016

LGTM! Test failure is unrelated.

@jart

This comment has been minimized.

Member

jart commented Dec 1, 2016

Mr. Jenkins: test this please

@jart

jart approved these changes Dec 1, 2016

Thank you for putting the time and effort into making this configuration maintainable for us. As a result, the TensorBoard team will put forth the effort to keep them in a working state if Jenkins reports to us that they're broken. That is a promise we'll make you. But we only promise that up until the point when we get our canonical Bazel build to serve all your needs.

@vit-stepanovs

This comment has been minimized.

Contributor

vit-stepanovs commented Dec 1, 2016

It looks like test failures are unrelated...

@gunan

This comment has been minimized.

Member

gunan commented Dec 1, 2016

For good measure, let me rerun tests, than we can merge right away.
Jenkins, test this please.

@gunan gunan merged commit 4612e88 into tensorflow:master Dec 1, 2016

10 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details
cla/google All necessary CLAs are signed

mrry added a commit to mrry/tensorflow that referenced this pull request Dec 1, 2016

Merge pull request tensorflow#5844 from vit-stepanovs/cmake_tensorboa…
…rd_deps

[CMake] Add TensorBoard dependencies to PIP package.

decentralion added a commit that referenced this pull request Dec 1, 2016

Merge pull request #5844 from vit-stepanovs/cmake_tensorboard_deps (#…
…6020)

[CMake] Add TensorBoard dependencies to PIP package.

@vit-stepanovs vit-stepanovs deleted the vit-stepanovs:cmake_tensorboard_deps branch Dec 1, 2016

@DomenicD

This comment has been minimized.

DomenicD commented Dec 8, 2016

How do I install this new PIP package or how can I create/build one with this fix?

@vit-stepanovs

This comment has been minimized.

Contributor

vit-stepanovs commented Dec 8, 2016

@DomenicD, for now you can build a PIP package manually by following these instructions. Otherwise, it looks like this fix will be included in 0.12rc1 release, which may be available some time soon.

@ferrouswheel

This comment has been minimized.

ferrouswheel commented Dec 8, 2016

@DomenicD if you don't want to build manually you can also get a whl of the nightly build from here: http://ci.tensorflow.org/view/Nightly/job/nightly-win/lastSuccessfulBuild/DEVICE=gpu,OS=windows/

@DomenicD

This comment has been minimized.

DomenicD commented Dec 14, 2016

@ferrouswheel thank you, nightly worked great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment