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

Add a workspace name #51

Merged
merged 1 commit into from
May 2, 2016
Merged

Add a workspace name #51

merged 1 commit into from
May 2, 2016

Conversation

kchodorow
Copy link
Contributor

@kchodorow kchodorow commented Apr 22, 2016

Bazel is going to be restructuring the runfiles tree (see
https://github.com/bazelbuild/bazel/wiki/Updating-the-runfiles-tree-structure
for details), which requires all projects to have a workspace name
(__main__ will be used if one is not provided).

Feel free to suggest a different name if you guys prefer, but something
needs to be set or the tests will break when Bazel starts using the new
runfiles tree structure.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@vinuraja
Copy link
Contributor

@tensorflow-jenkins: test this please

@kirilg
Copy link
Contributor

kirilg commented Apr 26, 2016

Thanks! Looks good, could you please squash commits?

@kchodorow
Copy link
Contributor Author

Done.

@kirilg
Copy link
Contributor

kirilg commented Apr 26, 2016

@tensorflow-jenkins: test this please

@kirilg
Copy link
Contributor

kirilg commented Apr 26, 2016

Seems Jenkins fails to find half_plus_two in the new directory. Is $(@D) correctly set with the new workspace name?

"cp -r /tmp/half_plus_two/* $(@D)/half_plus_two",

Could not find base path: /var/lib/jenkins/workspace/serving-pull-request-cpu/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/workspace/bazel-out/local_linux-fastbuild/bin/tensorflow_serving/servables/tensorflow/simple_servers_test.runfiles/org_tensorflow_serving/tensorflow_serving/session_bundle/example/half_plus_two

@kchodorow
Copy link
Contributor Author

Ugh, I think I know what this is. Bazel isn't properly noticing that the runfiles directory has changed. I might have to make a fix Bazel before this can be pushed, let me verify.

@kirilg
Copy link
Contributor

kirilg commented Apr 27, 2016

@tensorflow-jenkins: test this please

@kchodorow
Copy link
Contributor Author

GitHub hasn't gotten the fix yet (although it should in a few minutes).

@kirilg
Copy link
Contributor

kirilg commented Apr 27, 2016

I thought the only fix needed is to run 'bazel clean' so I updated our Jenkins config. We're still using the predefined bazel version (0.2.0) so we won't pull in changes to Github (to bazel if that's what you meant), or did you mean you updated the PR and just haven't pushed the changes to github?

@kchodorow
Copy link
Contributor Author

Ah, I thought you were pulling from Bazel's head. Cool! This should work, then, if it wiped out the existing output directories.

@kirilg
Copy link
Contributor

kirilg commented Apr 27, 2016

@tensorflow-jenkins: test this please

#include "tensorflow/core/platform/test.h"

namespace tensorflow {
namespace serving {
namespace test_util {

string TestSrcDirPath(const string& relative_path) {
string base_path = tensorflow::io::JoinPath(
Copy link
Contributor

Choose a reason for hiding this comment

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

const string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kirilg
Copy link
Contributor

kirilg commented Apr 28, 2016

@tensorflow-jenkins: test this please

@@ -1,3 +1,5 @@
workspace(name = "org_tensorflow_serving")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks so much for your patience with this. After some more thought, can we make this workspace name "tf_serving"? This simplifies some things for us and keeps the name short.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workspace names are supposed to be in reverse domain name form, so I don't think tf_serving really makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked with @damienmg and we decided to encourage but not enforce this in Bazel projects. Changed.

Bazel is going to be restructuring the runfiles tree (see
https://github.com/bazelbuild/bazel/wiki/Updating-the-runfiles-tree-structure)
for details), which requires all projects to have a workspace name
(__main__ will be used if one is not provided).

Feel free to suggest a different name if you guys prefer, but something
needs to be set or the tests will break when Bazel starts using the new
runfiles tree structure.

Change workspace name to org_tensorflow_serving
@kirilg kirilg merged commit de6d4e2 into tensorflow:master May 2, 2016
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.

5 participants