Skip to content

Conversation

@yf225
Copy link
Contributor

@yf225 yf225 commented Sep 19, 2019

With this PR, we establish the following conventions:

  1. Options in C++ module / optimizer constructors should always be const SomeOptions& type, not SomeOptions type.
  2. The options constructor arg should always be named options_, not options, to not be confused with the module / optimizer's internal field options.
  3. We never use std::move to assign options_ to the module / optimizer's internal field options in the constructor definition. Instead, we simply use options(options_).

Here is the reasoning:
We might be tempted to declare the constructor as SomeModule(SomeOptions options_) and have options(std::move(options_)) in the member initialization list. However, this can be a dangerous design because the constructor might use options_ to set values for other member fields in the member initialization list (e.g.

explicit LBFGS(ParameterContainer&& parameters, const LBFGSOptions& options)
: LossClosureOptimizer(std::forward<ParameterContainer>(parameters)),
options(options),
ro(options.history_size_),
al(options.history_size_) {}
), and use-after-move can cause hard-to-debug problems.
Instead, we choose to explicitly use const SomeOptions& type for options_, and never use std::move to assign it to the internal options field. This way we have stronger guarantee on the validity of options_ at any point in the constructor.

Notable exceptions to the above conventions:

  1. C++ Embedding module doesn't adhere to the conventions now, which will be fixed after Implement torch.nn.Embedding / EmbeddingBag in PyTorch C++ API #26358 is landed.
  2. C++ dataloader and dataset classes likely need similar changes. We will do it when we start to work on dataloader/dataset parity.

Thanks @ShahriarSS for discovering the options usage inconsistency! 🚀

@pytorchbot pytorchbot added the module: cpp Related to C++ API label Sep 19, 2019
@yf225
Copy link
Contributor Author

yf225 commented Sep 19, 2019

@pytorchbot rebase this please

@ShahriarRezghi
Copy link

@yf225 the code looks good to me but there are 22 CI errors. Just one thing: Can you please move nn::Fold's constructor to .cpp to be consistent with the rest of the modules?

@yf225
Copy link
Contributor Author

yf225 commented Sep 20, 2019

@pytorchbot rebase this please

@yf225
Copy link
Contributor Author

yf225 commented Sep 20, 2019

@ShahriarSS I believe the CI error should be unrelated (I am re-running CI to validate). I also made the changes to nn::Fold's constructor.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

@facebook-github-bot
Copy link
Contributor

@yf225 merged this pull request in 49777e6.

mingbowan pushed a commit to mingbowan/pytorch that referenced this pull request Sep 23, 2019
Summary:
With this PR, we establish the following conventions:
1. Options in C++ module / optimizer constructors should always be `const SomeOptions&` type, not `SomeOptions` type.
2. The options constructor arg should always be named `options_`, not `options`, to not be confused with the module / optimizer's internal field `options`.
3. We never use `std::move` to assign `options_` to the module / optimizer's internal field `options` in the constructor definition. Instead, we simply use `options(options_)`.

Here is the reasoning:
We might be tempted to declare the constructor as `SomeModule(SomeOptions options_)` and have `options(std::move(options_))` in the member initialization list. However, this can be a dangerous design because the constructor might use `options_` to set values for other member fields in the member initialization list (e.g. https://github.com/pytorch/pytorch/blob/8317f75b79fb78ceeeb928aa23a901d57274b9e1/torch/csrc/api/include/torch/optim/lbfgs.h#L30-L34), and use-after-move can cause hard-to-debug problems.
Instead, we choose to explicitly use `const SomeOptions&` type for `options_`, and never use `std::move` to assign it to the internal `options` field. This way we have stronger guarantee on the validity of `options_` at any point in the constructor.

Notable exceptions to the above conventions:
1. C++ Embedding module doesn't adhere to the conventions now, which will be fixed after pytorch#26358 is landed.
2. C++ dataloader and dataset classes likely need similar changes. We will do it when we start to work on dataloader/dataset parity.

Thanks ShahriarSS for discovering the options usage inconsistency! 🚀
Pull Request resolved: pytorch#26483

Differential Revision: D17500451

Pulled By: yf225

fbshipit-source-id: 49361a3519e4ede933789db75731d40144f0b617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: cpp Related to C++ API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants