C++/Python API parity for Conv{1,2,3}d layers, and add F::conv{1,2,3}d functionals #28917

yf225 · 2019-10-30T20:58:47Z

This PR changes the implementation of C++ Conv{1,2,3}d layers to exactly match the Python version, and add F::conv{1,2,3}d functionals. For more thorough testing, I will rely on the parity test mechanism which uses values from common_nn.py to generate the inputs and options that we are interested in testing.

This PR is BC-breaking in the following way:

In Conv{1,2,3}dOptions:

with_bias is renamed to bias.
input_channels is renamed to in_channels.
output_channels is renamed to out_channels.
The value of transposed doesn't affect the behavior of Conv{1,2,3}d layers anymore. Users should migrate their code to use ConvTranspose{1,2,3}d layers instead. Note that ConvTranspose{1,2,3}d cannot be used in a Sequential module because Sequential module doesn't support modules with forward method that can take optional arguments. Users should create their own wrapper for ConvTranspose{1,2,3}d and have its forward method just accept a tensor, if they want to use it in a Sequential module.
For example:

struct ConvTranspose2dWrapperImpl : public torch::nn::ConvTranspose2dImpl {
  using torch::nn::ConvTranspose2dImpl::ConvTranspose2dImpl;

  torch::Tensor forward(const torch::Tensor& input) {
    return torch::nn::ConvTranspose2dImpl::forward(input, c10::nullopt);
  }
};

TORCH_MODULE(ConvTranspose2dWrapper);

torch::nn::Sequential sequential(
  ConvTranspose2dWrapper(torch::nn::ConvTranspose2dOptions(3, 3, 4));

yf225 · 2019-10-30T23:03:50Z

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

+      int64_t output_channels,
+      ExpandingArray<1> kernel_size)
+      : Conv1dImpl(ConvOptions<1>(input_channels, output_channels, kernel_size)) {
+  }


I moved the constructor that takes input_channels, output_channels, kernel_size into each of the Conv{1,2,3}d subclasses, so that they can call each dimension's specific constructor that takes options (which contains specialized logic). This mirrors the design of the Python version.

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

pbelevich · 2019-10-31T17:00:08Z

torch/csrc/api/src/nn/modules/conv.cpp


+Conv1dImpl::Conv1dImpl(
+    ConvOptions<1> options_)
+    : ConvImpl(std::move(options_).transposed(false).output_padding(0)) {}


What's the reason to use std::move here?

It is because we pass options_ by value here, as a result we can use std::move as an optimization. The reason for passing options_ by value is that we want to be able to change options_ in-place (updating its transposed_ and output_padding_ fields) before passing it to the ConvImpl constructor.

Correct me if I'm wrong, but std::move() casts options_ to rvalue-reference, but I don't see any move-ctor or move assignment operators that can accept rvalue-references, neither in ConvOptions nor in ConvImpl

The move-ctor for ConvOptions will be created by the compiler automatically. Per https://en.cppreference.com/w/cpp/language/move_constructor:

If no user-defined move constructors are provided for a class type (struct, class, or union), and all of the following is true:
~ there are no user-declared copy constructors;
~ there are no user-declared copy assignment operators;
~ there are no user-declared move assignment operators;
~ there are no user-declared destructors;
then the compiler will declare a move constructor as a non-explicit inline public member of its class with the signature T::T(T&&).

ConvImpl has a constructor that accepts const ConvOptions<D>&, and we can pass an rvalue by const reference (https://stackoverflow.com/questions/36102728/why-is-it-allowed-to-pass-r-values-by-const-reference-but-not-by-normal-referenc) :D

pbelevich · 2019-10-31T17:00:59Z

torch/csrc/api/src/nn/modules/conv.cpp


+Conv2dImpl::Conv2dImpl(
+    ConvOptions<2> options_)
+    : ConvImpl(std::move(options_).transposed(false).output_padding(0)) {}


What's the reason to use std::move here?

It is because we pass options_ by value here, as a result we can use std::move as an optimization. The reason for passing options_ by value is that we want to be able to change options_ in-place (updating its transposed_ and output_padding_ fields) before passing it to the ConvImpl constructor.

pbelevich · 2019-10-31T17:01:05Z

torch/csrc/api/src/nn/modules/conv.cpp


+Conv3dImpl::Conv3dImpl(
+    ConvOptions<3> options_)
+    : ConvImpl(std::move(options_).transposed(false).output_padding(0)) {}


What's the reason to use std::move here?

It is because we pass options_ by value here, as a result we can use std::move as an optimization. The reason for passing options_ by value is that we want to be able to change options_ in-place (updating its transposed_ and output_padding_ fields) before passing it to the ConvImpl constructor.

pbelevich · 2019-10-31T18:09:00Z

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


 /// Options for a `D`-dimensional convolution module.
 template <size_t D>
 struct ConvOptions {


Sometimes we have TORCH_API before classname, sometimes not...

Right this is actually intentional - in https://github.com/pytorch/pytorch/blob/master/CONTRIBUTING.md#windows-development-tips:

However, there is one important counterexample to this principle: if you want a templated function to be instantiated at the call site, do NOT mark it with *_API (if you do mark it, you'll have to explicitly instantiate all of the specializations used by the call sites.)

Although we do explicitly instantiate all the specializations of ConvOptions in torch/csrc/api/src/nn/options/conv.cpp, I think it might still be a good idea to not put TORCH_API here, which is also consistent with how other templatized Options are implemented.

torch/csrc/api/src/nn/modules/conv.cpp

kostmo · 2019-10-31T19:06:53Z

CircleCI build failures summary

As of commit 7b1928c:

9/9 failures introduced in this PR
0/9 recognized as flaky

Here are the reasons each build failed:

Job	Step	Log excerpt
pytorch_linux_xenial_py3_6_gcc5_4_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-749j85ij/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_py2_7_9_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-i5YY9i/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_py3_clang5_asan_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-p0u75rcb/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_cuda9_cudnn7_py3_nogpu_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-emu3mj_x/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_xla_linux_xenial_py3_6_clang7_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-6uh4sdpl/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_cuda9_cudnn7_py3_NO_AVX_NO_AVX2_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-wcrgw6uj/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_cuda9_cudnn7_py3_slow_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-m6gj8v3w/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_cuda9_cudnn7_py3_NO_AVX2_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-y107hrvp/torchvision/csrc/models/googlenet.cpp:15:43: error:`
pytorch_linux_xenial_cuda9_cudnn7_py3_test	Test	`\'vision::models::_googlenetimpl::BasicConv2dImpl::BasicConv2dImpl(torch::nn::Conv2dOptions)\':\n/tmp/pip-req-build-7ktb6o2i/torchvision/csrc/models/googlenet.cpp:15:43: error:`

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 20 time(s).

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot · 2019-11-13T23:12:35Z

@yf225 merged this pull request in b37c235.

yf225 added the module: cpp Related to C++ API label Oct 30, 2019

yf225 requested review from ebetica and goldsborough as code owners October 30, 2019 20:58

yf225 force-pushed the fix_conv branch 2 times, most recently from cf623a4 to 7fa2034 Compare October 30, 2019 21:05

yf225 removed request for ebetica and goldsborough October 30, 2019 21:05

yf225 force-pushed the fix_conv branch 12 times, most recently from 6496e0e to 5caa26e Compare October 30, 2019 22:48

C++/Python API parity for Conv{1,2,3}d layers

9402cc4

yf225 force-pushed the fix_conv branch from 5caa26e to 9402cc4 Compare October 30, 2019 22:49

yf225 commented Oct 30, 2019

View reviewed changes

yf225 requested a review from pbelevich October 30, 2019 23:10

pbelevich reviewed Oct 31, 2019

View reviewed changes

Will Feng added 4 commits October 31, 2019 14:43

fix space

5238310

clang-tidy fix

6e8f840

reuse options

4a3321b

can't reuse options for some callsites

09dfa29

add FIXME to unbreak torchvision

480c6fe

yf225 added the module: bc-breaking Related to a BC-breaking change label Oct 31, 2019

yf225 requested a review from pbelevich October 31, 2019 19:30

yf225 mentioned this pull request Nov 1, 2019

Python/C++ API Parity: torch.nn modules and functional #25883

Open

pbelevich approved these changes Nov 1, 2019

View reviewed changes

Merge branch 'master' into fix_conv

54b17d7

facebook-github-bot reviewed Nov 13, 2019

View reviewed changes

yf225 force-pushed the fix_conv branch from b1c2280 to 5865454 Compare November 13, 2019 04:15

new options design

a8b6fdc

yf225 force-pushed the fix_conv branch from 5865454 to a8b6fdc Compare November 13, 2019 04:22

facebook-github-bot reviewed Nov 13, 2019

View reviewed changes

fix clang-tidy

7b1928c

facebook-github-bot reviewed Nov 13, 2019

View reviewed changes

yf225 changed the title ~~C++/Python API parity for Conv{1,2,3}d layers~~ C++/Python API parity for Conv{1,2,3}d layers, and add F::conv{1,2,3}d functionals Nov 13, 2019

more bridges

3f67457

facebook-github-bot reviewed Nov 13, 2019

View reviewed changes

facebook-github-bot closed this in b37c235 Nov 13, 2019

facebook-github-bot added the merged label Nov 13, 2019

yf225 mentioned this pull request Nov 14, 2019

Rename with_bias() to bias(), and output_channels() to out_channels() in C++ conv layer options usage pytorch/vision#1576

Merged

yf225 mentioned this pull request Nov 23, 2019

[C++ API] Rename with_bias to bias in conv options pytorch/examples#669

Closed

yf225 mentioned this pull request Dec 9, 2019

C++ dcgan example failing on generator #30931

Closed

mruberry added the Merged label Oct 28, 2020

C++/Python API parity for Conv{1,2,3}d layers, and add F::conv{1,2,3}d functionals #28917

C++/Python API parity for Conv{1,2,3}d layers, and add F::conv{1,2,3}d functionals #28917

Uh oh!

Conversation

yf225 commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 Nov 1, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kostmo commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CircleCI build failures summary

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!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yf225 commented Oct 30, 2019 •

edited

Loading

yf225 Nov 1, 2019 •

edited

Loading

kostmo commented Oct 31, 2019 •

edited

Loading