Unify C++ API with C++ extensions #11510

goldsborough · 2018-09-11T04:55:27Z

Currently the C++ API and C++ extensions are effectively two different, entirely orthogonal code paths. This PR unifies the C++ API with the C++ extension API by adding an element of Python binding support to the C++ API. This means the torch/torch.h included by C++ extensions, which currently routes to torch/csrc/torch.h, can now be rerouted to torch/csrc/api/include/torch/torch.h -- i.e. the main C++ API header. This header then includes Python binding support conditioned on a define (TORCH_WITH_PYTHON_BINDINGS), which is only passed when building a C++ extension.

Currently stacked on top of #11498

Why is this useful?

One less codepath. In particular, there has been trouble again and again due to the two torch/torch.h header files and ambiguity when both ended up in the include path. This is now fixed.
I have found that it is quite common to want to bind a C++ API module back into Python. This could be for simple experimentation, or to have your training loop in Python but your models in C++. This PR makes this easier by adding pybind11 support to the C++ API.
The C++ extension API simply becomes richer by gaining access to the C++ API headers.

@soumith @ezyang @apaszke

fmassa

Nice!

I have a few questions regarding model serialization in Python.
It might be worth looking into also adding equivalents of load_state_dict and state_dict maybe? Also, I have the impression that a cpp module extension will not be seen by the python nn.Module as a nn.Module, so it might not recurse into it, is that right?

torch/csrc/api/include/torch/python.h

fmassa · 2018-09-11T09:16:15Z

More generally, would it be possible (or worth it) making the cpp module extensions (when loaded in Python) inherit from nn.Module? Or would that require that nn.Module is moved entirely to Python?

goldsborough · 2018-09-11T18:12:46Z

@fmassa That's a good and important question! The definite answer here, however, is that Python API and C++ API modules are currently not inter-operable at all, and so making one a submodule of the other will not work and is not intended to work right now. There could be future work of unifying them, but right now nn::Module and nn.Module are completely different things and cannot be mixed. In the future we may be able to give them a common C++ base class, along with JIT ScriptModule's too.

My intention here is simply to make it easier to add Python bindings to the C++ API, but not to make the C++ API inter-operate better with the Python API. The use case here is that you have a C++ API model and would like to do something with it in Python for experimentation, for which C++ extensions have been super quick in my personal attempts at this. Another use case (which we have internally) is that you could keep two copies of your model, one in the Python API which you use for quick experiments, and one in the C++ API which you use in prod, and then have your training loop, data wrangling and visualization in Python, and instantiate either the C++ or Python model dynamically inside Python and all your training loop code will work on both.

What this unification does give to C++ extension users is the utilities the C++ API provides, that is simple things like torch::cuda::is_available() or the torch::NoGradGuard (equivalent to with torch.no_grad()). So it's not super revolutionary in that sense.

For us developers it is useful because we no longer have to think of the C++ extension codebase (currently torch/csrc/torch.h) and the C++ API codebase as separate.

fmassa · 2018-09-11T18:35:32Z

@goldsborough I see, thanks for the explanation!
If we document exposing torch::nn into Python, it might be worth mentioning that we are not supposed to use the C++ Extension nn modules inside python nn modules then.

torch/csrc/api/include/torch/serialization.h

goldsborough · 2018-09-14T07:35:28Z

@ezyang could you review this?

torch/utils/cpp_extension.py

ezyang · 2018-09-14T14:39:48Z

C++ looks ok, but I'm concerned about the macro.

facebook-github-bot

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

facebook-github-bot

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

More unification Remove ../ include in one C++ extension Solve the constructor problem for ModuleHolder Create extension.h and deprecate torch/torch.h for C++ extensions Include serialize/*.h in conda package Skip cpp_api_extension test on Windows Small improvements

facebook-github-bot

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

Summary: Currently the C++ API and C++ extensions are effectively two different, entirely orthogonal code paths. This PR unifies the C++ API with the C++ extension API by adding an element of Python binding support to the C++ API. This means the `torch/torch.h` included by C++ extensions, which currently routes to `torch/csrc/torch.h`, can now be rerouted to `torch/csrc/api/include/torch/torch.h` -- i.e. the main C++ API header. This header then includes Python binding support conditioned on a define (`TORCH_WITH_PYTHON_BINDINGS`), *which is only passed when building a C++ extension*. Currently stacked on top of pytorch/pytorch#11498 Why is this useful? 1. One less codepath. In particular, there has been trouble again and again due to the two `torch/torch.h` header files and ambiguity when both ended up in the include path. This is now fixed. 2. I have found that it is quite common to want to bind a C++ API module back into Python. This could be for simple experimentation, or to have your training loop in Python but your models in C++. This PR makes this easier by adding pybind11 support to the C++ API. 3. The C++ extension API simply becomes richer by gaining access to the C++ API headers. soumith ezyang apaszke Pull Request resolved: pytorch/pytorch#11510 Reviewed By: ezyang Differential Revision: D9998835 Pulled By: goldsborough fbshipit-source-id: 7a94b44a9d7e0377b7f1cfc99ba2060874d51535

goldsborough requested review from apaszke, colesbury, ebetica, ezyang, fmassa, gchanan, soumith and zdevito as code owners September 11, 2018 04:55

fmassa reviewed Sep 11, 2018

View reviewed changes

dzhulgakov reviewed Sep 11, 2018

View reviewed changes

torch/csrc/api/include/torch/serialization.h Outdated

This comment was marked as off-topic.

Sign in to view

goldsborough force-pushed the cpp-ext-api branch 7 times, most recently from ea34e65 to 0718236 Compare September 14, 2018 04:56

ezyang reviewed Sep 14, 2018

View reviewed changes

torch/utils/cpp_extension.py Outdated

This comment was marked as off-topic.

Sign in to view

goldsborough force-pushed the cpp-ext-api branch 3 times, most recently from 23b1df1 to 917708d Compare September 21, 2018 03:51

facebook-github-bot reviewed Sep 21, 2018

View reviewed changes

ezyang approved these changes Sep 22, 2018

View reviewed changes

goldsborough force-pushed the cpp-ext-api branch 2 times, most recently from 2863bca to 09d619f Compare September 22, 2018 14:59

facebook-github-bot reviewed Sep 22, 2018

View reviewed changes

goldsborough added 2 commits September 22, 2018 17:23

Add C++ frontend include to TorchConfig.cmake

ddce1a9

goldsborough force-pushed the cpp-ext-api branch from 09d619f to ddce1a9 Compare September 23, 2018 00:25

facebook-github-bot reviewed Sep 23, 2018

View reviewed changes

facebook-github-bot closed this in e05d689 Sep 24, 2018

alsrgv mentioned this pull request Sep 26, 2018

PyTorch 1.0 readiness updates horovod/horovod#523

Merged

ezyang added the merged label Jun 26, 2019

Unify C++ API with C++ extensions #11510

Unify C++ API with C++ extensions #11510

Uh oh!

Conversation

goldsborough commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

fmassa commented Sep 11, 2018

Uh oh!

goldsborough commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmassa commented Sep 11, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

goldsborough commented Sep 14, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

ezyang commented Sep 14, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

goldsborough commented Sep 11, 2018 •

edited

Loading

goldsborough commented Sep 11, 2018 •

edited

Loading