[c10d] Split custom class bindings out of python binding code #58992

suo · 2021-05-26T16:09:08Z

Stack from ghstack:

[c10d] Split custom class bindings out of python binding code #58992 [c10d] Split custom class bindings out of python binding code

Currently, we define Torchbind custom classes in the same place that we define Python bindings.

This is nice from a code location perspective, but has two downsides:

These custom classes are not available in a C++-only build.
These break when included in torch::deploy.

Some explanation on the second issue: torch::deploy creates many Python
interpreters, and creates a full copy of all the bindings for each one. This
will run the static initialization code once for each copy of the bindings,
leading to multiple registration of the custom classes (and therefore an
error).

This PR splits out the relevant custom class binding code into its own source
file to be included in libc10d, which can be compiled and statically
initialized a single time and linked against from the c10d python bindings.

Differential Revision: D28690832

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

Differential Revision: D28690832

Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! [ghstack-poisoned]

facebook-github-bot · 2021-05-26T16:09:11Z

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job	Step	Action
^{pytorch_linux_bionic_py3_6_clang9_noarch_test}	^{Run tests}	🔁 rerun

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! ghstack-source-id: 129959421 Pull Request resolved: #58992

…ode" Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832) [ghstack-poisoned]

suo · 2021-05-27T15:58:32Z

@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…ode" Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832) [ghstack-poisoned]

Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! ghstack-source-id: dba676a Pull Request resolved: #58992

suo · 2021-05-27T16:04:05Z

@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

torch/lib/c10d/CMakeLists.txt

…ode" Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832) [ghstack-poisoned]

suo · 2021-05-28T03:35:42Z

@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…ode" Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832) [ghstack-poisoned]

Pull Request resolved: #58992 Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! ghstack-source-id: 130152256

…ode" Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832) [ghstack-poisoned]

Pull Request resolved: #58992 Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! ghstack-source-id: 32fa680

suo · 2021-05-28T04:30:20Z

@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

suo · 2021-05-28T04:30:51Z

@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…ode" Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832) [ghstack-poisoned]

Pull Request resolved: #58992 Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. Differential Revision: [D28690832](https://our.internmc.facebook.com/intern/diff/D28690832/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28690832/)! ghstack-source-id: 130168942

suo · 2021-05-28T17:25:53Z

torch/lib/c10d/frontend.cpp

  pg_names_[process_group] = name;
 }

+void initCustomClassBindings() {


I was struggling to get the linker not throw out these global static initializers on all platforms, so I gave up and made the initialization explicit, which seems like better practice anyway. Currently it's being initialized when we initialize the rest of the Python bindings (e.g. from libtorch_python), but since the actual binding code is in libc10d we can initialize it from other places (like libtorch_deploy).

suo · 2021-05-28T17:26:46Z

torch/lib/c10d/CMakeLists.txt


 if(USE_C10D_NCCL)
-  target_compile_definitions(c10d INTERFACE USE_C10D_NCCL)
+  target_compile_definitions(c10d PUBLIC USE_C10D_NCCL)


For posterity, these have been changed from INTERFACE to PUBLIC because now we also use these compile definitions in the implementation of c10d itself (e.g. checking USE_C10D_NCCL in frontend.cpp to conditionally initialize nccl process group bindings).

facebook-github-bot · 2021-05-28T22:37:19Z

This pull request has been merged in b977a3b.

…h#58992) Summary: Pull Request resolved: pytorch#58992 Currently, we define Torchbind custom classes in the same place that we define Python bindings. This is nice from a code location perspective, but has two downsides: 1. These custom classes are not available in a C++-only build. 2. These break when included in torch::deploy. Some explanation on the second issue: torch::deploy creates many Python interpreters, and creates a full copy of all the bindings for each one. This will run the static initialization code once for each copy of the bindings, leading to multiple registration of the custom classes (and therefore an error). This PR splits out the relevant custom class binding code into its own source file to be included in libc10d, which can be compiled and statically initialized a single time and linked against from the c10d python bindings. ghstack-source-id: 130168942 Test Plan: CI Reviewed By: wconstab Differential Revision: D28690832 fbshipit-source-id: 3c5e3fff28abb8bcdb4a952794c07de1ee2ae5a8

suo requested review from H-Huang, mingzhe09088, mrshenli, pritamdamania87, rohan-varma, wayi1 and zhaojuanmao as code owners May 26, 2021 16:09

facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels May 26, 2021

suo commented May 27, 2021

View reviewed changes

torch/lib/c10d/CMakeLists.txt Outdated Show resolved Hide resolved

suo commented May 28, 2021

View reviewed changes

facebook-github-bot closed this in b977a3b May 28, 2021

facebook-github-bot added the Merged label May 28, 2021

facebook-github-bot deleted the gh/suo/437/head branch June 1, 2021 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[c10d] Split custom class bindings out of python binding code #58992

[c10d] Split custom class bindings out of python binding code #58992

Uh oh!

suo commented May 26, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented May 26, 2021 •

edited

Loading

Uh oh!

suo commented May 27, 2021

Uh oh!

suo commented May 27, 2021

Uh oh!

Uh oh!

suo commented May 28, 2021

Uh oh!

suo commented May 28, 2021

Uh oh!

suo commented May 28, 2021

Uh oh!

suo May 28, 2021

Uh oh!

suo May 28, 2021

Uh oh!

facebook-github-bot commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[c10d] Split custom class bindings out of python binding code #58992

[c10d] Split custom class bindings out of python binding code #58992

Uh oh!

Conversation

suo commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

1 failure not recognized by patterns:

Uh oh!

suo commented May 27, 2021

Uh oh!

suo commented May 27, 2021

Uh oh!

Uh oh!

suo commented May 28, 2021

Uh oh!

suo commented May 28, 2021

Uh oh!

suo commented May 28, 2021

Uh oh!

suo May 28, 2021

Choose a reason for hiding this comment

Uh oh!

suo May 28, 2021

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

suo commented May 26, 2021 •

edited

Loading

facebook-github-bot commented May 26, 2021 •

edited

Loading