-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Fix options usage in C++ module / optimizer constructors #26483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@pytorchbot rebase this please |
|
@yf225 the code looks good to me but there are 22 CI errors. Just one thing: Can you please move |
|
@pytorchbot rebase this please |
|
@ShahriarSS I believe the CI error should be unrelated (I am re-running CI to validate). I also made the changes to |
facebook-github-bot
left a comment
There was a problem hiding this 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.
vincentqb
left a comment
There was a problem hiding this 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!
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
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! 🚀