[C++ API] Make Sequential ref-counted #9151

goldsborough · 2018-07-03T22:54:59Z

In the C++ API, Sequential currently was not refcounted itself, but stored shared_ptr<AnyModule> to get the reference semantics. This is unfortunate because most modules in the API are accessed via ->, e.g. Linear l(1, 2); l->forward(...);. Sequential was different in that it had value semantics itself, thus was accessed via ..

This PR makes Sequential store AnyModule (without extra indirection), and uses the same pImpl mechanism we use for all other modules to make Sequential have reference semantics itself. This makes it consistent with the rest of the library. It also removes one level of indirection inside of Sequential, which is cool.

One thing I had to change was that the ModuleHolder with which the whole pImpl thing is implemented previously did some tricks to make Linear(3, 4) actually construct Linear(LinearOptions(3, 4)). This doesn't work well with Sequential since it takes a variadic parameter pack. Instead, I made ModuleHolder forward all arguments to the underlying module, and then further pushed the trick to forward parameters to modules' options types into the actual Modules. This adds one constructor per Module in the library. This is not something user modules have to do (unless they want this nice forwarding themselves). It makes the code simpler overall.

@ezyang @ebetica @apaszke

test/cpp/api/module.cpp

@@ -191,7 +191,7 @@ TEST_CASE("module/clone") {
        buffer = register_buffer("buf", torch::ones({2, 2}));
      }

-      Linear l1, l2, l3;
+      Linear l1{nullptr}, l2{nullptr}, l3{nullptr};


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

+  /// Constructs the `ModuleHolder` with a contained module, forwarding all
+  /// arguments to its constructor.
+  template <typename... Ts>
+  explicit ModuleHolder(Ts&&... ts)


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

-  class Name : public torch::nn::ModuleHolder<Impl> {  \
-   public:                                             \
-    using torch::nn::ModuleHolder<Impl>::ModuleHolder; \
-    using torch::nn::ModuleHolder<Impl>::operator->;   \


torch/csrc/api/include/torch/nn/modules/any.h

@@ -315,6 +322,16 @@ template <typename ModuleType>
 AnyModule::AnyModule(const ModuleHolder<ModuleType>& module_holder)
    : AnyModule(module_holder.get()) {}

+inline AnyModule::AnyModule(const AnyModule& other)
+    : content_(other.content_ ? other.content_->clone() : nullptr) {}


goldsborough · 2018-07-05T20:15:23Z

@pytorchbot retest this please

apaszke

(Not a complete review)

test/cpp/api/sequential.cpp

-          REQUIRE(sequential[i].get() == modules[i].get());
-          REQUIRE(sequential.ptr<M>(i).get() == modules[i].get());
+          REQUIRE(sequential->ptr(i).get() == modules[i].get());
+          REQUIRE((*sequential)[i].get() == modules[i].get());


goldsborough · 2018-07-09T17:10:55Z

Can we get closure on this PR? @ezyang @apaszke did you have more concerns?

facebook-github-bot

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

Summary: In the C++ API, `Sequential` currently was not refcounted itself, but stored `shared_ptr<AnyModule>` to get the reference semantics. This is unfortunate because most modules in the API are accessed via `->`, e.g. `Linear l(1, 2); l->forward(...);`. `Sequential` was different in that it had value semantics itself, thus was accessed via `.`. This PR makes `Sequential` store `AnyModule` (without extra indirection), and uses the same pImpl mechanism we use for all other modules to make `Sequential` have reference semantics itself. This makes it consistent with the rest of the library. It also removes one level of indirection inside of `Sequential`, which is cool. One thing I had to change was that the `ModuleHolder` with which the whole pImpl thing is implemented previously did some tricks to make `Linear(3, 4)` actually construct `Linear(LinearOptions(3, 4))`. This doesn't work well with `Sequential` since it takes a variadic parameter pack. Instead, I made `ModuleHolder` forward all arguments to the underlying module, and then further pushed the trick to forward parameters to modules' options types into the actual Modules. This adds one constructor per Module in the library. This is not something user modules have to do (unless they want this nice forwarding themselves). It makes the code simpler overall. ezyang ebetica apaszke Pull Request resolved: pytorch#9151 Reviewed By: ezyang Differential Revision: D8809298 Pulled By: goldsborough fbshipit-source-id: da68452c3de912fbc67af330ba93b5220de6909f

goldsborough requested review from apaszke, colesbury, ebetica, ezyang, gchanan, soumith and zdevito as code owners July 3, 2018 22:54

ezyang reviewed Jul 4, 2018

View reviewed changes

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

/// Constructs the `ModuleHolder` with a contained module, forwarding all

/// arguments to its constructor.

template <typename... Ts>

explicit ModuleHolder(Ts&&... ts)

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Jul 4, 2018

View reviewed changes

torch/csrc/api/include/torch/nn/pimpl.h Outdated

class Name : public torch::nn::ModuleHolder<Impl> { \

public: \

using torch::nn::ModuleHolder<Impl>::ModuleHolder; \

using torch::nn::ModuleHolder<Impl>::operator->; \

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Jul 4, 2018

View reviewed changes

goldsborough force-pushed the seq-pimpl branch from 074f929 to ae66974 Compare July 5, 2018 19:25

apaszke reviewed Jul 6, 2018

View reviewed changes

goldsborough added 2 commits July 6, 2018 15:19

Improve pimpl mechanism and Sequential

d97f999

Post rebase fixes

2dbedce

goldsborough force-pushed the seq-pimpl branch from 1df7d40 to 2dbedce Compare July 6, 2018 22:20

ezyang approved these changes Jul 11, 2018

View reviewed changes

facebook-github-bot reviewed Jul 11, 2018

View reviewed changes

facebook-github-bot closed this in 153e2e9 Jul 12, 2018

ezyang added the merged label Jun 26, 2019

[C++ API] Make Sequential ref-counted #9151

[C++ API] Make Sequential ref-counted #9151

Uh oh!

Conversation

goldsborough commented Jul 3, 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!

goldsborough commented Jul 5, 2018

Uh oh!

apaszke 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!

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

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goldsborough commented Jul 9, 2018 •

edited

Loading