[WIP] C++ API by goldsborough · Pull Request #6345 · pytorch/pytorch

goldsborough · 2018-04-06T08:17:05Z

This PR will be the development ground of PyTorch's C++ API for a while. To get CI feedback, without pushing to master, I will be posting continuous updates into this PR, one isolated, squashed commit at a time, and request reviews of individual commits.

CC @ebetica @jgehring

goldsborough · 2018-04-06T08:17:35Z

To begin, this PR creates a prototypical C++ API for torch::nn::Module. Lots of things are not yet implemented, however I created a stub (which compiles) for an LSTM module that showcases the general look and feel of the API, and have documented the methods I think torch::nn::Module should have. This PR also sets up the general directory structure, which I have chosen as torch/csrc/api as a start. I have the build system updates in another PR that I will push here once ready.

dzhulgakov · 2018-04-06T17:59:43Z

This is cool! Just making sure you're aware of https://sourcegraph.com/github.com/pytorch/pytorch/-/blob/torch/csrc/jit/script/module.h - we need to make sure that there's a single nn.Module abstraction in C++ which works with python, script, tracing, etc

torch/csrc/api/include/torch/nn/module.h

torch/csrc/api/include/torch/nn/rnn.h

torch/csrc/api/include/torch/tensor.h

torch/csrc/api/src/module.cpp

torch/csrc/api/include/torch/nn/rnn.h

torch/csrc/api/include/torch/nn/module.h

apaszke · 2018-04-08T21:37:47Z

Nice POC! I found a few worrying things and left comments

jgehring

This is perhaps a naive question, but why provide a clone() method rather than a copy constructor? clone() requires CRTP if you want to return the correct type, and in the current implementation it dictates the storage for the resulting module (unique_ptr).

goldsborough · 2018-04-09T15:35:56Z

It's for when all you have is an abstract Module*, i.e. a "polymorphic copy constructor". In that case you need to know the static type to invoke the copy constructor. @ebetica was showing me some neuroevolution stuff where you guys needed this

ebetica · 2018-04-09T16:33:38Z

@jgehring see the virtual copy constructor paradigm:

https://www.geeksforgeeks.org/advanced-c-virtual-copy-constructor/

jgehring · 2018-04-09T17:45:39Z

Ah thanks, of course, I completely missed that. Indeed, we discussed CRTP quite a lot for @ebetica's autogradpp...

goldsborough · 2018-04-10T20:23:55Z

I'v squashed my commits a bit to now have:

Initial commit + operator() (already reviewed)
Stuff to make the code compile with cpp_build, so that CI is now building my changes
Code move to put the OrderedDict data structure from torch/csrc/jit/module.cpp into the API folder
Implemented the Sequential container (reviews welcome)

setup.py

 include_dirs += [
    cwd,
    os.path.join(cwd, "torch", "csrc"),
+    os.path.join(cwd, "torch", "csrc", "api", "include"),


torch/csrc/api/include/torch/nn/module.h

torch/csrc/jit/script/module.h

+    if (auto method = methods.find(name)) {
+      return method->get();
+    }
+    return nullptr;


torch/csrc/jit/script/module.h

 #include "torch/csrc/autograd/variable.h"
-#include <ATen/optional.h>
+
+#include "torch/csrc/api/include/torch/detail/ordered_dict.h"


torch/csrc/api/include/torch/nn/sequential.h

+    append(std::forward<Tail>(tail)...);
+  }
+
+  std::vector<std::unique_ptr<Module>> modules_;


torch/csrc/api/include/torch/nn/sequential.h

+
+  /// Adds a new `Module` to the `Sequential` container.
+  template <typename M>
+  void append(M&& module) {


ezyang · 2018-04-12T03:36:30Z

CC @smessmer who is our resident C++ expert

tools/cpp_build/libtorch/CMakeLists.txt

  message(STATUS "ATEN_BUILD_PATH is ${ATEN_BUILD_PATH}")
 endif()

+set(CMAKE_CXX_FLAGS "--std=c++11 -Wall -Wextra -Wno-unused-parameter -Wno-unused-variable ${CMAKE_CXX_FLAGS}")


goldsborough · 2018-04-12T03:56:12Z

Changes:

OrderedDict now stores Items, to support iteration (otherwise there's no easy way to go from a value back to its key)
Submodules are stored as shared_ptrs instead of Module*.
Implementation of cursors, which allow hierarchical traversal of submodules, parameters and buffers. Essentially a wrapper over std::vector<std::pair<std::string, Module>> but with a nice API to be able to write e.g. module.parameters().find("key") or module.modules.apply([](Module& module) { ... }). -- @smessmer may want to have a look
Quick hacky implementation of zero_grad() based on the cursors API. Need to think more about the Variable API, and implement toType_()

apaszke

Mostly LGTM. Let's get this in soon, because it's getting dangerously large.

tools/cpp_build/libtorch/CMakeLists.txt

+
+## API tests
+
+SET(TORCH_API_TEST_SRCS ${TORCH_SRC_DIR}/csrc/api/test/test.cpp)


tools/cpp_build/libtorch/CMakeLists.txt

+  "${TORCH_SRC_DIR}/../aten/src/ATen/utils/catch/single_include"
+  "${COMMON_INCLUDES}")
+
+INSTALL(TARGETS test_api


torch/csrc/api/include/torch/nn/cursor.h

+  size_t size() const noexcept;
+
+ protected:
+  std::vector<Item> items_;


torch/csrc/api/src/nn/module.cpp

+
+void Module::type(at::ScalarType new_type) {
+  // parameters().apply([=](Tensor& tensor) { tensor.toType_(new_type); });
+  // buffers().apply([=](Tensor& tensor) { tensor.toType_(new_type); });


torch/csrc/jit/script/module.h

 #include "torch/csrc/autograd/variable.h"
-#include <ATen/optional.h>
+
+#include "torch/csrc/api/include/torch/detail/ordered_dict.h"


torch/csrc/jit/script/module.h

+    if (auto method = methods.find(name)) {
+      return method->get();
+    }
+    return nullptr;


torch/csrc/jit/script/init.cpp

-
+        py::tuple result(self.get_parameters().size());
+        size_t index = 0;
+        for (auto& parameter : self.get_parameters()) {


torch/csrc/jit/script/init.cpp

+        for (auto& method : self.get_methods()) {
+          method_names.push_back((*method)->name());
+        }
+        return method_names;


goldsborough · 2018-04-15T20:41:48Z

@apaszke I was actually planning on doing all development in this PR, and squashing commits to make it manageable. This means we don't have to make it "public" yet (so people don't use it). And I think if I push it onto a branch, say pytorch:cpp , the CI won't test it when I make PRs (is this the case?)? Is this a good idea?

apaszke · 2018-04-15T20:52:10Z

I think it's easier to merge this thing in parts, even if it's going to undergo breaking changes. It's already 1,1k lines long, and reviewing PRs that have 2k+ is really hard.

It's probably ok to merge this even before it's public. I doubt it's going to be used before we actually start advertising it, and we should make it clear that it's really experimental and will break your code.

apaszke

Just to make sure this isn't merged before you remove the accidental ONNX submodule change.

goldsborough · 2018-04-30T20:05:27Z

Will be rebased on top of autogradpp

goldsborough requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners April 6, 2018 08:17

goldsborough assigned zdevito, apaszke and colesbury Apr 6, 2018

onnxbot-worker-2 mentioned this pull request Apr 6, 2018

[auto] pytorch-pr-6345 onnxbot/onnx-fb-universe#1452

Closed

dzhulgakov self-requested a review April 6, 2018 17:56

dzhulgakov requested a review from jamesr66a April 6, 2018 17:59

apaszke reviewed Apr 8, 2018

View reviewed changes

jgehring reviewed Apr 9, 2018

View reviewed changes

goldsborough force-pushed the cpp branch from 1590823 to d31d8ff Compare April 10, 2018 20:02

goldsborough added 4 commits April 10, 2018 13:21

Initial commit of C++ Module

ae9d7b9

Add API directories to cpp_build and make them build in CI

50686b4

Put OrderedDict into torch/csrc/api

907d951

Implement Sequential container

cae76cd

goldsborough force-pushed the cpp branch from d31d8ff to cae76cd Compare April 10, 2018 20:21

apaszke reviewed Apr 11, 2018

View reviewed changes

goldsborough added 2 commits April 11, 2018 20:05

Make Sequential store shared_ptrs

3298677

Made OrderedDict item based

ca29da5

goldsborough added 2 commits April 11, 2018 20:06

Implement cursors

6b0da15

Implement zero_grad

53b73fb

ezyang reviewed Apr 12, 2018

View reviewed changes

goldsborough added 2 commits April 11, 2018 20:59

Replace torch::detail:: with detail::

71ee9ba

Use const char* for name

0b7918b

goldsborough mentioned this pull request Apr 12, 2018

Pytorch in java #6570

Closed

apaszke reviewed Apr 15, 2018

View reviewed changes

apaszke suggested changes Apr 15, 2018

View reviewed changes

This was referenced Apr 28, 2018

C++ API masterthread #7073

Closed

Merge autogradpp into PyTorch #7074

Merged

goldsborough closed this Apr 30, 2018

goldsborough mentioned this pull request May 7, 2018

Rename Container to Module in autogradpp and reorg code #7304

Merged

goldsborough mentioned this pull request May 16, 2018

[C++ API] Implement builder style construction #7597

Merged

goldsborough mentioned this pull request May 24, 2018

[C++ API] Improve and use OrderedDict for parameters / modules #7823

Merged

goldsborough mentioned this pull request Jun 6, 2018

[C++ API] Cursors #8190

Merged


		## API tests

		SET(TORCH_API_TEST_SRCS ${TORCH_SRC_DIR}/csrc/api/test/test.cpp)

Conversation

goldsborough commented Apr 6, 2018

Uh oh!

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

Uh oh!

Uh oh!

dzhulgakov commented Apr 6, 2018

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!

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!

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!

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!

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!

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!

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!

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!

apaszke commented Apr 8, 2018

Uh oh!

jgehring left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goldsborough commented Apr 9, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

goldsborough commented Apr 6, 2018 •

edited

Loading

jgehring left a comment •

edited

Loading

goldsborough commented Apr 9, 2018 via email •

edited

Loading

goldsborough commented Apr 10, 2018 •

edited

Loading