Fix options usage in C++ module / optimizer constructors#26483
Closed
yf225 wants to merge 5 commits intopytorch:masterfrom
Closed
Fix options usage in C++ module / optimizer constructors#26483yf225 wants to merge 5 commits intopytorch:masterfrom
yf225 wants to merge 5 commits intopytorch:masterfrom
Conversation
Contributor
Author
|
@pytorchbot rebase this please |
|
@yf225 the code looks good to me but there are 22 CI errors. Just one thing: Can you please move |
Contributor
Author
|
@pytorchbot rebase this please |
Contributor
Author
|
@ShahriarSS I believe the CI error should be unrelated (I am re-running CI to validate). I also made the changes to |
Contributor
facebook-github-bot
left a comment
There was a problem hiding this comment.
@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
vincentqb
approved these changes
Sep 20, 2019
Contributor
vincentqb
left a comment
There was a problem hiding this comment.
This looks good to me!
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With this PR, we establish the following conventions:
const SomeOptions&type, notSomeOptionstype.options_, notoptions, to not be confused with the module / optimizer's internal fieldoptions.std::moveto assignoptions_to the module / optimizer's internal fieldoptionsin the constructor definition. Instead, we simply useoptions(options_).Here is the reasoning:
We might be tempted to declare the constructor as
SomeModule(SomeOptions options_)and haveoptions(std::move(options_))in the member initialization list. However, this can be a dangerous design because the constructor might useoptions_to set values for other member fields in the member initialization list (e.g.pytorch/torch/csrc/api/include/torch/optim/lbfgs.h
Lines 30 to 34 in 8317f75
Instead, we choose to explicitly use
const SomeOptions&type foroptions_, and never usestd::moveto assign it to the internaloptionsfield. This way we have stronger guarantee on the validity ofoptions_at any point in the constructor.Notable exceptions to the above conventions:
Thanks @ShahriarSS for discovering the options usage inconsistency! 🚀