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

[WIP] Add bazel runfiles manifest support for Windows #15475

Closed
wants to merge 1 commit into from
Closed

[WIP] Add bazel runfiles manifest support for Windows #15475

wants to merge 1 commit into from

Conversation

snnn
Copy link
Contributor

@snnn snnn commented Dec 19, 2017

To implement the ideas in:
https://groups.google.com/forum/#!msg/bazel-discuss/Po8xN8dhWkI/sWPUYV9YBAAJ

Now //tensorflow/core:example_example_parser_configuration_test is disabled on Windows, because it cannot find the data files.

testing::RunFileRelocator::GetInstance().Relocate() is the replacement of testing::TensorFlowSrcRoot(). We should mark TensorFlowSrcRoot as deprecated or completely remove it.

Places need to change:
c/c_api_test.cc
cc/saved_model/loader_test.cc
compiler/aot/codegen_test.cc
compiler/xla/service/gpu/llvm_gpu_backend/utils_test.cc
compiler/xla/tests/sample_file_test.cc
contrib/ffmpeg/default/ffmpeg_lib_test.cc
contrib/lite/models/test_utils.h
contrib/lite/testing/generated_examples_zip_test.cc
contrib/session_bundle/bundle_shim_test.cc
contrib/session_bundle/test_util.cc
core/distributed_runtime/rpc/grpc_testlib.cc
core/grappler/costs/graph_properties_test.cc
core/grappler/utils/scc_test.cc
core/kernels/hexagon/graph_transferer_test.cc
core/kernels/spectrogram_test.cc
core/platform/cloud/google_auth_provider_test.cc
core/profiler/internal/tfprof_show_test.cc
core/profiler/internal/tfprof_stats_test.cc
core/profiler/internal/tfprof_tensor_test.cc
core/profiler/internal/tfprof_timeline_test.cc
core/example/example_parser_configuration_test.cc
core/platform/cloud/oauth_client_test.cc

Ref: bazelbuild/bazel#4215

TODO:

  1. deal with folders
  2. do not translate abs path.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@snnn
Copy link
Contributor Author

snnn commented Dec 19, 2017

@laszlocsomor
Should we switch to use "RUNFILES_DIR" env on Linux? Or continue to use TEST_SRCDIR/TEST_WORKSPACE.

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Dec 19, 2017

@snnn :

@laszlocsomor
Should we switch to use "RUNFILES_DIR" env on Linux? Or continue to use TEST_SRCDIR/TEST_WORKSPACE.

Yes, I recommend using $RUNFILES_DIR.

Wait, actually, let me double check that... :) sorry

@snnn
Copy link
Contributor Author

snnn commented Dec 19, 2017

Hi @laszlocsomor
I have two more questions:

  1. Paths contain spaces
    I saw there is a TODO at
    https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/windows/runfiles/WindowsRunfiles.java#L54
    What's the current status of this?

  2. The first column in manifest file starts with "org_tensorflow/tensorflow/" . Now I hardcode this string in tensorflow's c++ code. Is it ok? How can I get this string from ... somewhere?

@laszlocsomor
Copy link
Contributor

I don't think it matters which one you use.

Tests conventionally use TEST_SRCDIR. Its value is either the same as RUNFILES_DIR, or is set to a language-specific runfiles directory value such as JAVA_RUNFILES, which ironically is again the same as RUNFILES_DIR.

@laszlocsomor
Copy link
Contributor

  1. Paths contain spaces
    I saw there is a TODO at
    https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/windows/runfiles/WindowsRunfiles.java#L54
    What's the current status of this?

AFAIK nobody fixed it yet. Thanks for raising my awareness to it! Filed bazelbuild/bazel#4327, I'll fix it in Q1-2018.

  1. The first column in manifest file starts with "org_tensorflow/tensorflow/" . Now I hardcode this string in tensorflow's c++ code. Is it ok? How can I get this string from ... somewhere?

The first segment "org_tensorflow" is the name of the repository. If it's the main workspace, its name is in $TEST_WORKSPACE. If it's an external repo, then its name is the name you gave the repository rule in the WORKSPACE file.

The second segment is already part of the path within the workspace or repository.

@snnn
Copy link
Contributor Author

snnn commented Dec 20, 2017

Hi TF team,

In order to make these cc_tests work on Windows, I need to touch a lot of source files. Should I submit them one by one, or , combine all these changes in one PR?

Now this PR contains one change to the test framework, and two unitest fixes for

core/example/example_parser_configuration_test.cc
core/platform/cloud/oauth_client_test.cc

@snnn
Copy link
Contributor Author

snnn commented Dec 20, 2017

Hi @laszlocsomor

Current implementation has a limit: it only has file to file mappings, no folder to folder mappings. Say, If we have a function which takes a folder as input, e.g.

model.h

void LoadModel(const string& model_dir);

And we want to test this function.

model_test.cc

TEST(Model, LoadModelFromLocalDir) {
   string dirname = "data/checkpoint1"
   dirname = TranslateDirNameByUsingManifestInfo(dirname); //This step is not do-able
   LoadModel(dirname); 
}

It's not do-able. Because the path, "data/checkpoint1", doesn't exist in the MANIFEST file, which only has things like:

data/checkpoint1/aaa/bbb.txt        C:/xxx/aaa/bbb.txt
data/checkpoint1/aaa/ccc/ddd.txt        C:/xxx/aaa/ccc/ddd.txt

There is no entry for "data/checkpoint1"

@laszlocsomor
Copy link
Contributor

@snnn : Sorry I don't follow, what has a limitation? Do you mean that the runfiles manifest doesn't allow enumerating runfiles directories, or something else?

@snnn
Copy link
Contributor Author

snnn commented Dec 20, 2017

@laszlocsomor: Yes. I don't know how to relocate a dir.

@snnn snnn changed the title Add bazel runfiles manifest support for Windows [WIP] Add bazel runfiles manifest support for Windows Dec 20, 2017
@laszlocsomor
Copy link
Contributor

@snnn : I see. I filed bazelbuild/bazel#4334. In the meantime, can you use find on Linux, and grep on Windows?

@snnn
Copy link
Contributor Author

snnn commented Dec 20, 2017

Yes, I can. Then?

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Dec 20, 2017

Then until bazelbuild/bazel#4334 is fixed, use find/grep to work around the missing feature of listing directories, like so:

  • on Linux:

    find "$RUNFILES_DIR/repository_name/path/to/dir" -not -type d
    
  • on Windows:

    grep '^repository_name/path/to/dir' "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-
    

@snnn
Copy link
Contributor Author

snnn commented Jan 3, 2018

Have no time to work on this right now. Close it temporarily.

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.

None yet

4 participants