Skip to content

Fetch TORCH_SRCS from build_variables.bzl#36737

Closed
malfet wants to merge 1 commit intopytorch:masterfrom
malfet:malfet/cmake-to-use-build-variable.bzl
Closed

Fetch TORCH_SRCS from build_variables.bzl#36737
malfet wants to merge 1 commit intopytorch:masterfrom
malfet:malfet/cmake-to-use-build-variable.bzl

Conversation

@malfet
Copy link
Copy Markdown
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
Share build file lists between CMake, Bazel and Buck builds #35456
Closed
@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented Apr 16, 2020 •
edited
Loading

💊 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.

@malfet
Fetch TORCH_SRCS from build_variables.bzl
7446cc7 
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
orionr
orionr approved these changes Apr 16, 2020
View reviewed changes
Copy link
Copy Markdown
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!

facebook-github-bot
facebook-github-bot reviewed Apr 16, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

dzhulgakov commented Apr 17, 2020

@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
Copy Markdown
Contributor Author

malfet commented Apr 17, 2020 •
edited
Loading

@dzhulgakov you are absolutely right. Addressed in #36809

ljk53
ljk53 reviewed Apr 22, 2020
View reviewed changes
Comment thread caffe2/CMakeLists.txt
${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
Copy Markdown
Contributor

@ljk53 ljk53 Apr 22, 2020

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
Copy Markdown
Contributor Author

@malfet malfet Apr 22, 2020

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.

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@malfet @facebook-github-bot
Fetch TORCH_SRCS from build_variables.bzl (pytorch#36737)
7ea3fb4 
Summary:
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`
Pull Request resolved: pytorch#36737

Test Plan: CI

Differential Revision: D21078753

Pulled By: malfet

fbshipit-source-id: f46ca48d48aa122188f028136c14687ff52629ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kostmo kostmo Awaiting requested review from kostmo

@dzhulgakov dzhulgakov Awaiting requested review from dzhulgakov

3 more reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ljk53 ljk53 ljk53 left review comments

@orionr orionr orionr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Share build file lists between CMake, Bazel and Buck builds

5 participants

@malfet @dzhulgakov @orionr @facebook-github-bot @ljk53