Skip to content

absorb THD into main cmake build#12775

Closed
anderspapitto wants to merge 1 commit intopytorch:masterfrom
anderspapitto:build-libs
Closed

absorb THD into main cmake build#12775
anderspapitto wants to merge 1 commit intopytorch:masterfrom
anderspapitto:build-libs

Conversation

@anderspapitto
Copy link
Copy Markdown
Contributor

@anderspapitto anderspapitto commented Oct 17, 2018 •
edited
Loading

We want to move _C into the same cmake invocation that builds
libcaffe2 and libtorch. However, _C depends on THD and c10d, which in
turn depend on libcaffe2. That means that we can't move _C into that
cmake file unless we do these two first. This change does so.

@anderspapitto anderspapitto force-pushed the build-libs branch 6 times, most recently from c8255d6 to 3c9e61e Compare October 17, 2018 20:27
@anderspapitto anderspapitto changed the title [WIP] move THD into main cmake build [WIP] absorb THD and c10d into main cmake build, kill PREFIX_PATH Oct 17, 2018
apaszke
apaszke reviewed Oct 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Won't this make the build much slower?

@anderspapitto
Copy link
Copy Markdown
Contributor Author

anderspapitto commented Oct 17, 2018

@apaszke why so?

@anderspapitto anderspapitto force-pushed the build-libs branch 4 times, most recently from 744bbc9 to 8cf4666 Compare October 17, 2018 20:49
@apaszke
Copy link
Copy Markdown
Contributor

apaszke commented Oct 17, 2018

Because so far those were built as part of the build_deps step which you could skip almost every time you tried to rebuild PyTorch while developing.

@anderspapitto
Copy link
Copy Markdown
Contributor Author

anderspapitto commented Oct 17, 2018

@apaszke that's unaffected. THD and c10d are still part of build_deps after this change.

@anderspapitto anderspapitto force-pushed the build-libs branch 3 times, most recently from 36b1d33 to 7221b3d Compare October 17, 2018 22:47
@anderspapitto anderspapitto changed the title [WIP] absorb THD and c10d into main cmake build, kill PREFIX_PATH absorb THD and c10d into main cmake build, kill PREFIX_PATH Oct 17, 2018
@anderspapitto
Copy link
Copy Markdown
Contributor Author

anderspapitto commented Oct 17, 2018

I'm still tracking down one bug (in (only) some of our linux CI environments, linking c10d/example fails to find -lgloo and -lgloo_cuda). In the meantime this diff is more-or-less ready for review

soumith
soumith previously requested changes Oct 18, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@soumith soumith left a comment

Choose a reason for hiding this comment

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

see inline comments.

Main things to be fixed:

  • dont manually list out all the include_directories, directly depend on caffe2 (and possibly caffe2_gpu?
  • you very likely have to move nccl and gloo building into cmake in this PR as well, or have to debug some nasty side-effects

Comment thread setup.py Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread tools/build_pytorch_libs.sh Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/lib/THD/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/lib/THD/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/lib/c10d/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

@anderspapitto anderspapitto changed the title absorb THD and c10d into main cmake build absorb THD into main cmake build Oct 19, 2018
@anderspapitto anderspapitto force-pushed the build-libs branch 6 times, most recently from 64ff793 to 17e894f Compare October 23, 2018 17:31
@anderspapitto anderspapitto force-pushed the build-libs branch 5 times, most recently from 2a3d41c to 3ba678c Compare October 24, 2018 20:42
@anderspapitto anderspapitto dismissed stale reviews from ezyang and soumith October 25, 2018 00:53

addressed

soumith
soumith requested changes Oct 25, 2018
View reviewed changes
Comment thread torch/lib/THD/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/lib/THD/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/lib/THD/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/lib/THD/CMakeLists.txt Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@soumith soumith mentioned this pull request Oct 25, 2018
Closed
@anderspapitto anderspapitto mentioned this pull request Oct 25, 2018
Closed
soumith
soumith approved these changes Oct 25, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@soumith soumith left a comment

Choose a reason for hiding this comment

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

approved based on further investigations. Gloo is only used in THD when it's built with CUDA support.

facebook-github-bot
facebook-github-bot reviewed Oct 25, 2018
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.

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

@anderspapitto anderspapitto deleted the build-libs branch October 25, 2018 04:33
@soumith soumith mentioned this pull request Oct 30, 2018
Closed
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@anderspapitto @facebook-github-bot
absorb THD into main cmake build (pytorch#12775)
0197be1 
Summary:
We want to move _C into the same cmake invocation that builds
libcaffe2 and libtorch. However, _C depends on THD and c10d, which in
turn depend on libcaffe2. That means that we can't move _C into that
cmake file unless we do these two first. This change does so.
Pull Request resolved: pytorch#12775

Differential Revision: D10457374

Pulled By: anderspapitto

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

Reviewers

@soumith soumith soumith approved these changes

@ezyang ezyang ezyang left review comments

@pietern pietern Awaiting requested review from pietern

@teng-li teng-li Awaiting requested review from teng-li

@zdevito zdevito Awaiting requested review from zdevito

@ebetica ebetica Awaiting requested review from ebetica

@goldsborough goldsborough Awaiting requested review from goldsborough

+3 more reviewers

@Yangqing Yangqing Yangqing left review comments

@apaszke apaszke apaszke left review comments

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@anderspapitto @apaszke @ezyang @Yangqing @soumith @facebook-github-bot