Skip to content

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Apr 16, 2020

Mimic .bzl parsing logic from pytorch/FBGEMM#344
Generate libtorch_cmake_sources by running following script:

def read_file(path):
    with open(path) as f:
        return f.read()

def get_cmake_torch_srcs():
    caffe2_cmake = read_file("caffe2/CMakeLists.txt")
    start = caffe2_cmake.find("set(TORCH_SRCS")
    end = caffe2_cmake.find(")", start)
    return caffe2_cmake[start:end+1]
def get_cmake_torch_srcs_list():
    caffe2_torch_srcs = get_cmake_torch_srcs()
    unfiltered_list = [x.strip() for x in get_cmake_torch_srcs().split("\n") if len(x.strip())>0]
    return [x.replace("${TORCH_SRC_DIR}/","torch/") for x in unfiltered_list if 'TORCH_SRC_DIR' in x]

import imp
build_variables = imp.load_source('build_variables', 'tools/build_variables.bzl')
libtorch_core_sources = set(build_variables.libtorch_core_sources)
caffe2_torch_srcs = set(get_cmake_torch_srcs_list())
if not libtorch_core_sources.issubset(caffe2_torch_srcs):
    print("libtorch_core_sources must be a subset of caffe2_torch_srcs")
print(sorted(caffe2_torch_srcs.difference(libtorch_core_sources)))

Move common files between libtorch_cmake_sources and libtorch_extra_sources to libtorch_jit_core_sources

Test Plan: CI

@malfet malfet requested review from dzhulgakov, kostmo and orionr April 16, 2020 18:55
@malfet malfet linked an issue Apr 16, 2020 that may be closed by this pull request
@dr-ci
Copy link

dr-ci bot commented Apr 16, 2020

💊 Build failures summary and remediations

As of commit 7446cc7 (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

XLA failure

Job pytorch_xla_linux_bionic_py3_6_clang9_build is failing. Please create an issue with title prefixed by [PT_BREAK] in pytorch/xla and link to to this PR. If you have questions, please reach out to @ailzhang / @dlibenzi / @JackCaoG.


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 8 times.

Mimic `.bzl` parsing logic from pytorch/FBGEMM#344
Generate `libtorch_cmake_sources` by running following script:
```
def read_file(path):
    with open(path) as f:
        return f.read()

def get_cmake_torch_srcs():
    caffe2_cmake = read_file("caffe2/CMakeLists.txt")
    start = caffe2_cmake.find("set(TORCH_SRCS")
    end = caffe2_cmake.find(")", start)
    return caffe2_cmake[start:end+1]
def get_cmake_torch_srcs_list():
    caffe2_torch_srcs = get_cmake_torch_srcs()
    unfiltered_list = [x.strip() for x in get_cmake_torch_srcs().split("\n") if len(x.strip())>0]
    return [x.replace("${TORCH_SRC_DIR}/","torch/") for x in unfiltered_list if 'TORCH_SRC_DIR' in x]

import imp
build_variables = imp.load_source('build_variables', 'tools/build_variables.bzl')
libtorch_core_sources = set(build_variables.libtorch_core_sources)
caffe2_torch_srcs = set(get_cmake_torch_srcs_list())
if not libtorch_core_sources.issubset(caffe2_torch_srcs):
    print("libtorch_core_sources must be a subset of caffe2_torch_srcs")
print(sorted(caffe2_torch_srcs.difference(libtorch_core_sources)))
```

Move common files between `libtorch_cmake_sources` and `libtorch_extra_sources` to `libtorch_jit_core_sources`

Test Plan: CI
@malfet malfet force-pushed the malfet/cmake-to-use-build-variable.bzl branch from 94524dd to 7446cc7 Compare April 16, 2020 20:22
Copy link
Contributor

@orionr orionr left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dzhulgakov
Copy link
Collaborator

@malfet - maybe I missed it, but don't you need add_dependencies call to make sure that when build_variables.bzl changes CMake get rerun?

@malfet malfet deleted the malfet/cmake-to-use-build-variable.bzl branch April 17, 2020 15:46
@malfet
Copy link
Contributor Author

malfet commented Apr 17, 2020

@dzhulgakov you are absolutely right. Addressed in #36809

${TORCH_SRC_DIR}/csrc/jit/api/function_impl.cpp
${TORCH_SRC_DIR}/csrc/jit/runtime/vararg_functions.cpp

${TORCH_SRC_DIR}/csrc/jit/tensorexpr/bounds_inference.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

@malfet do you have plan to read other lists (those gated by USE_DISTRIBUTED / USE_CUDA / etc) from build_variables.bzl as well?
I'm asking because I plan to remove 'tensorexpr/' from mobile build to reduce binary size. My plan is to create a separate list in build_variables.bzl and move tensorexpr/ related stuff from "libtorch_cmake_sources" into it. I'd like to make sure it's aligned with your long term plan before making the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that aligns with the longer term plan.
Please go ahead and create a list in build_variables.bzl and read it in caffe2/CmakeLists.txt using get_filelist() macro. And feel free to expand it to append_filelist, if it'll make more sense.

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.

Share build file lists between CMake, Bazel and Buck builds

5 participants