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

goldsborough · 2018-05-15T22:14:54Z

This PR implements our discussed "builder" style construction mechanism, where the class is fused with the builder itself. It is similar to the KWARGS mechanism in autogradpp, with some differences.

Largely, this PR:

Creates a macro TORCH_PARAMETER (name up for discussion, maybe TORCH_PROPERTY?) used to give modules parameters/properties. It is written in a way so that it requires only 2 arguments instead of 5, like AUTOGRAD_KWARG did
Rewrites core modules to follow a construction mechanism where
1. Required arguments go to the constructor,
2. Optional arguments are passed via setters/getters,
3. Variable construction and other heavy lifting is moved to a reset() function
4. reset() is called inside build(), which finalizes construction, and clone()
Does some heavy refactoring of the Convolution and RNN modules to better use polymorphism, e.g.:
1. In Conv, the call to at::conv<dimension>d(...) is now left to a virtual function, implemented in Conv1d, Conv2d and Conv3d
2. Large rewrite of rnn classes to also move their autograd implementations to virtual methods, and in general to avoid the use of the RNN mode beyond CuDNN. The RNN mode should not replace inheritance and polymorphism!!!

Recommended review order:

include/torch/nn/module.h
include/torch/nn/modules/*.h and src/nn/modules/*.cpp
tests

CC @ezyang @ebetica @apaszke @jgehring

ebetica

Reset parameters disappeared somewhere. I'm curious why we're departing from PyTorch in this department? I wonder why it's even there in the first place. @apaszke maybe you can shed some light on this?

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

+  variable_list CUDNN_forward(variable_list);
+  variable_list autograd_forward(variable_list);
+
+  void flatten_parameters_for_cudnn();


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

-  enum RNNMode { RNN_RELU = 0, RNN_TANH = 1, LSTM = 2, GRU = 3 };
+  // These must line up with the CUDNN mode codes:
+  // https://docs.nvidia.com/deeplearning/sdk/cudnn-developer-guide/index.html#cudnnRNNMode_t
+  enum class CuDNNMode { RNN_RELU, RNN_TANH, LSTM, GRU };


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

+  std::vector<Variable> hhb_;
+
+  size_t number_of_gates_;
+  bool has_cell_state_;


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

 }} // namespace torch::nn
+
+#define TORCH_PARAMETER(T, name)             \


ebetica

Okay besides my nits above.

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

+    return this->name##_;                    \
+  }                                          \
+                                             \
+ protected:                                  \


apaszke

I'm not sure why certain things changed as they are. The weight names are now inconsistent with those in Python, and I really don't think we should mess with visibility in our macros. The fact that it automagically changes just because you declared a parameter will be a constant source of unclear errors (the user doesn't have the line that makes things protected in their code!)

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

+  std::shared_ptr<Derived> build() {
+    auto module = std::make_shared<Derived>(static_cast<Derived&&>(*this));
+    module->reset();
+    return std::move(module);


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

+
+#define TORCH_PARAMETER(T, name)             \
+ public:                                     \
+  auto name(T new_##name)->decltype(*this) { \


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

+    return this->name##_;                    \
+  }                                          \
+                                             \
+ protected:                                  \


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

@@ -19,32 +19,32 @@ class ContainerListImpl : public CloneableModule<Derived> {
  }

  std::shared_ptr<Module> add(std::shared_ptr<Module> m) {
-    return append(m).children_.back();
+    return append(m).modules_.back();


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

-  enum RNNMode { RNN_RELU = 0, RNN_TANH = 1, LSTM = 2, GRU = 3 };
+  // These must line up with the CUDNN mode codes:
+  // https://docs.nvidia.com/deeplearning/sdk/cudnn-developer-guide/index.html#cudnnRNNMode_t
+  enum class CuDNNMode { RNN_RELU, RNN_TANH, LSTM, GRU };


test/cpp/api/container.cpp

    REQUIRE(model->param("test.l1.bias").size(0) == 3);
-    REQUIRE(model->param("test.l1.weight").size(0) == 3);
-    REQUIRE(model->param("test.l1.weight").size(1) == 10);
+    REQUIRE(model->param("test.l1.weights").size(0) == 3);


goldsborough · 2018-05-16T15:33:03Z

Sorry for the weight naming changes, I didn't know that's what they were called in Python. "weight" sounded more like a single float to me than a tensor of "weights", but I'll change it back.

As for the visibility of the value inside TORCH_PARAMETER: This one is tricky. I know messing with visibility labels is awkward and may cause trouble if users use it outside of core modules. Making all variables public I would like to avoid. Making the declaration separate will add boilerplate, since everything now has to be repeated twice... then we could have just as well gone with "separate builder style construction" (where the builder class is separate from the actual class) ...

ebetica · 2018-05-16T16:12:57Z

I actually think there's an argument to be made for making things as public as possible anyway, if we're to encourage hackability. I think not changing the visibility is more important and a bigger source of bugs than if people were to directly just use the private variables. The point of private variables, getters, and setters is that you can do arbitrary logic inside them, so you shouldn't touch the private variables, but presumably with a parameter macro like this we never worry about that issue at all.

ezyang · 2018-05-16T18:16:34Z

Yes, make 'em public! If the user really cares they can stop using the macro and write the getters/properties themselves.

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

-  std::unique_ptr<Module> clone() const override {
-    auto ptr = std::unique_ptr<Module>(
-        new Derived(*static_cast<const Derived*>(this)));
+  virtual void reset() = 0;


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

+    return std::move(module);
+  }
+
+  std::shared_ptr<Module> clone() const override {


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

-      bool transposed = false,
-      bool with_bias = true,
-      int groups = 1);
+  struct ExpandingSize {


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

+
+  variable_list forward(variable_list input) override;
+
+  TORCH_PARAMETER(double, rate) = 0.5;


ezyang · 2018-05-16T18:28:38Z

This is not the fault of this patch, but I was thinking about safety: shoudn't the parameters array should store Variable& and not Variable, so that if someone writes

mod.foo_ = my_new_var_param

the parameters array is updated properly?

goldsborough · 2018-05-16T19:40:55Z

@ezyang Oh man, the fact that mod.foo_ = my_new_var_param works just gives me shivers. Theoretically I would say no, because if you move a Module all your references become garbage. If we're really hardcore about only ever storing Modules inside shared_ptrs, it would work. But I don't think we can make that constraint for user modules

_

…construction (pytorch/pytorch#7597) pytorch/pytorch@cba19e5

* upstream/master: Makes AccumulateGrad high priority in backwards passes (pytorch#7604) [C++ API] Implement builder style construction (pytorch#7597) C10D: Added TCPStore to support C10D store interface (pytorch#7560) [auto] Update onnx to ba86ec2 - Protobuf typing (onnx/onnx#982) onnx/onnx@ba86ec2 Add LBFGS optimization algorithm to C++ API (pytorch#7596)

* Implemented fused builder based construction mechanism * "weights" -> "weight" * Use int64_t instead of size_t everywhere in RNN * Extracted Conv::ExpandingSize into its own thing * Rename TORCH_PARAMETER to TORCH_ATTR * Added documentation * Fix weight names in batchnorm module

goldsborough requested review from apaszke, colesbury, ebetica, ezyang, gchanan, soumith and zdevito as code owners May 15, 2018 22:14

o8ht88z00f mentioned this pull request May 15, 2018

[auto] pytorch-pr-7597 onnxbot/onnx-fb-universe#2089

Closed

ebetica reviewed May 15, 2018

View reviewed changes

ebetica approved these changes May 15, 2018

View reviewed changes

ebetica reviewed May 15, 2018

View reviewed changes

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

return this->name##_; \

} \

\

protected: \

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke previously requested changes May 16, 2018

View reviewed changes

goldsborough force-pushed the construction branch from 0028a70 to db43a50 Compare May 16, 2018 15:59

ezyang reviewed May 16, 2018

View reviewed changes

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

std::unique_ptr<Module> clone() const override {

auto ptr = std::unique_ptr<Module>(

new Derived(*static_cast<const Derived*>(this)));

virtual void reset() = 0;

This comment was marked as off-topic.

Sign in to view

ezyang reviewed May 16, 2018

View reviewed changes

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

return std::move(module);

}

std::shared_ptr<Module> clone() const override {

This comment was marked as off-topic.

Sign in to view

ezyang reviewed May 16, 2018

View reviewed changes

torch/csrc/api/include/torch/nn/modules/conv.h Outdated

bool transposed = false,

bool with_bias = true,

int groups = 1);

struct ExpandingSize {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed May 16, 2018

View reviewed changes

torch/csrc/api/include/torch/nn/modules/dropout.h Outdated

variable_list forward(variable_list input) override;

TORCH_PARAMETER(double, rate) = 0.5;

This comment was marked as off-topic.

Sign in to view

ezyang approved these changes May 16, 2018

View reviewed changes

goldsborough added 4 commits May 16, 2018 12:36

Implemented fused builder based construction mechanism

c25000d

"weights" -> "weight"

2940550

Use int64_t instead of size_t everywhere in RNN

d888008

Extracted Conv::ExpandingSize into its own thing

2226f3f

goldsborough force-pushed the construction branch from db43a50 to 2226f3f Compare May 16, 2018 19:36

goldsborough added 2 commits May 16, 2018 14:16

Rename TORCH_PARAMETER to TORCH_ATTR

1bfb5a8

Added documentation

fc5f134

goldsborough added 2 commits May 16, 2018 15:03

Fix weight names in batchnorm module

9669d21

Merge branch 'master' into construction

994d74d

goldsborough merged commit cba19e5 into pytorch:master May 17, 2018

goldsborough deleted the construction branch May 17, 2018 21:10

onnxbot added a commit to onnxbot/onnx-fb-universe that referenced this pull request May 17, 2018

[auto] Update pytorch to cba19e5 - [C++ API] Implement builder style …

e94f314

…construction (pytorch/pytorch#7597) pytorch/pytorch@cba19e5


		variable_list forward(variable_list input) override;

		TORCH_PARAMETER(double, rate) = 0.5;

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

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

Conversation

goldsborough commented May 15, 2018 • edited Loading

ebetica left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ebetica left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

apaszke left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

goldsborough commented May 16, 2018

ebetica commented May 16, 2018

ezyang commented May 16, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ezyang commented May 16, 2018

goldsborough commented May 16, 2018 • edited Loading

goldsborough commented May 15, 2018 •

edited

Loading

goldsborough commented May 16, 2018 •

edited

Loading