Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> #34468

pbelevich · 2020-03-09T02:34:09Z

This PR prepares at::Generator for pybind11's type_caster<at::Generator> which is required to implement custom RNG in python. The following changes are done:

at::Generator was moved to c10::GeneratorImpl (similar to c10::TensorImpl)
at::Generator was recreated as a holder of std::shared_ptr<c10::GeneratorImpl> (similar to at::Tensor that holds c10::intrusive_ptr<c10::TensorImpl>)
Most of at::Generator* usages were replaced with at::Generator

TBD: replacing Generator generator = nullptr with {} requires JIT changes(adding Generator to IValue?)

Stack from ghstack:

Move normal() to DistributionTemplates #35167 Move normal() to DistributionTemplates
pybind11 type_caster for at::Generator and custom RNG python test #34774 pybind11 type_caster for at::Generator and custom RNG python test
Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> #34468 Replace Generator with Generator that holds std::shared_ptr*

Differential Revision: D20549420

[ghstack-poisoned]

ghstack-source-id: dca28dd Pull Request resolved: #34468

dr-ci · 2020-03-09T03:53:42Z

💊 CircleCI build failures summary and remediations

As of commit 52b450d (more details on the Dr. CI page):

1/2 failures introduced in this PR
1/2 broken upstream at merge base f515d87 since Mar 21
Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)
```
If your commit is older than viable/strict:
```
git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD
```
Check out the recency history of this "viable master" tracking branch.

🕵️ 1 new failure recognized by patterns

The following build failures do not appear to be due to upstream breakages:

pytorch_xla_linux_xenial_py3_6_clang7_build (1/1)

Step: "Build" (full log | pattern match details)

Mar 21 14:56:19 torch_xla/csrc/aten_xla_type_default.cpp:11460:8: error: no matching member function for call to 'impl_unboxedOnlyKernel'

Mar 21 14:56:19       ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 
Mar 21 14:56:19 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/op_registration/op_registration.h:294:74: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'kernel_func' 
Mar 21 14:56:19     std::enable_if_t<guts::is_function_type<FuncType>::value, Options&&> impl_unboxedOnlyKernel(DispatchKey dispatch_key) && { 
Mar 21 14:56:19                                                                          ^ 
Mar 21 14:56:19 torch_xla/csrc/aten_xla_type_default.cpp:11199:8: error: no matching member function for call to 'impl_unboxedOnlyKernel' 
Mar 21 14:56:19       .impl_unboxedOnlyKernel<at::Tensor &(at::Tensor &, double, double, at::Generator), &AtenXlaType::normal_>(at::DispatchKey::XLATensorId) 
Mar 21 14:56:19       ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 
Mar 21 14:56:19 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/op_registration/op_registration.h:294:74: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'kernel_func' 
Mar 21 14:56:19     std::enable_if_t<guts::is_function_type<FuncType>::value, Options&&> impl_unboxedOnlyKernel(DispatchKey dispatch_key) && { 
Mar 21 14:56:19                                                                          ^ 
Mar 21 14:56:19 torch_xla/csrc/aten_xla_type_default.cpp:11460:8: error: no matching member function for call to 'impl_unboxedOnlyKernel' 
Mar 21 14:56:19       .impl_unboxedOnlyKernel<at::Tensor(const at::Tensor &, const at::Tensor &, at::Scalar, at::Scalar, bool, at::Generator), &AtenXlaType::rrelu_with_noise>(at::DispatchKey::XLATensorId) 
Mar 21 14:56:19       ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 
Mar 21 14:56:19 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/op_registration/op_registration.h:294:74: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'kernel_func' 
Mar 21 14:56:19     std::enable_if_t<guts::is_function_type<FuncType>::value, Options&&> impl_unboxedOnlyKernel(DispatchKey dispatch_key) && { 
Mar 21 14:56:19                                                                          ^ 
ackages/torch/include/torch/csrc/api/include -I/opt/conda/lib/python3.6/site-packages/torch/include/TH -I/opt/conda/lib/python3.6/site-packages/torch/include/THC -I/opt/conda/include/python3.6m -c torch_xla/csrc/batch_norm.cpp -o build/temp.linux-x86_64-3.6/torch_xla/csrc/batch_norm.o -std=c++14 -Wno-sign-compare -Wno-deprecated-declarations -Wno-return-type -Wno-macro-redefined -Wno-return-std-move -DNDEBUG -DTORCH_API_INCLUDE_EXTENSION_H -DTORCH_EXTENSION_NAME=_XLAC -D_GLIBCXX_USE_CXX11_ABI=1 
torch/include/torch/csrc/api/include -I/opt/conda/lib/python3.6/site-packages/torch/include/TH -I/opt/conda/lib/python3.6/site-packages/torch/include/THC -I/opt/conda/include/python3.6m -c torch_xla/csrc/layout_manager.cpp -o build/temp.linux-x86_64-3.6/torch_xla/csrc/layout_manager.o -std=c++14 -Wno-sign-compare -Wno-deprecated-declarations -Wno-return-type -Wno-macro-redefined -Wno-return-std-move -DNDEBUG -DTORCH_API_INCLUDE_EXTENSION_H -DTORCH_EXTENSION_NAME=_XLAC -D_GLIBCXX_USE_CXX11_ABI=1 
Mar 21 14:56:26 11 errors generated. 
on3.6/site-packages/torch/include/torch/csrc/api/include -I/opt/conda/lib/python3.6/site-packages/torch/include/TH -I/opt/conda/lib/python3.6/site-packages/torch/include/THC -I/opt/conda/include/python3.6m -c torch_xla/csrc/view.cpp -o build/temp.linux-x86_64-3.6/torch_xla/csrc/view.o -std=c++14 -Wno-sign-compare -Wno-deprecated-declarations -Wno-return-type -Wno-macro-redefined -Wno-return-std-move -DNDEBUG -DTORCH_API_INCLUDE_EXTENSION_H -DTORCH_EXTENSION_NAME=_XLAC -D_GLIBCXX_USE_CXX11_ABI=1 
te-packages/torch/include/torch/csrc/api/include -I/opt/conda/lib/python3.6/site-packages/torch/include/TH -I/opt/conda/lib/python3.6/site-packages/torch/include/THC -I/opt/conda/include/python3.6m -c torch_xla/csrc/nll_loss.cpp -o build/temp.linux-x86_64-3.6/torch_xla/csrc/nll_loss.o -std=c++14 -Wno-sign-compare -Wno-deprecated-declarations -Wno-return-type -Wno-macro-redefined -Wno-return-std-move -DNDEBUG -DTORCH_API_INCLUDE_EXTENSION_H -DTORCH_EXTENSION_NAME=_XLAC -D_GLIBCXX_USE_CXX11_ABI=1

🚧 1 upstream failure:

These were probably caused by upstream breakages:

pytorch_linux_xenial_py3_clang5_asan_test since Mar 21

This comment was automatically generated by Dr. CI (expand for details).

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 142 times.

[ghstack-poisoned]

ghstack-source-id: 1a8841c Pull Request resolved: #34468

[ghstack-poisoned]

ghstack-source-id: 5755613 Pull Request resolved: #34468

[ghstack-poisoned]

ghstack-source-id: 66906e6 Pull Request resolved: #34468

[ghstack-poisoned]

ghstack-source-id: cbe8580 Pull Request resolved: #34468

[ghstack-poisoned]

ghstack-source-id: 1108dee Pull Request resolved: #34468

[ghstack-poisoned]

ghstack-source-id: 6021401 Pull Request resolved: #34468

[ghstack-poisoned]

ghstack-source-id: 5b5418a Pull Request resolved: #34468

[ghstack-poisoned]

This PR adds c-tor `Generator(Device device)` to C++ API to match Python API, to avoid code duplication the logic was moved from `THPGenerator_pynew` to corresponding `at::Generator` c-tor. Unfortunately it makes Generator.h depend on CPUGenerator.h and CUDAGenerator.h, but this is how our Python API works :( * #34468 Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> [ghstack-poisoned]

…tr<GeneratorImpl>" This PR prepares `at::Generator` for pybind11's `type_caster<at::Generator>` which is required to implement custom RNG in python. The following changes are done: 1. `at::Generator` was moved to `c10::GeneratorImpl` (similar to `c10::TensorImpl`) 2. `at::Generator` was recreated as a holder of `std::shared_ptr<c10::GeneratorImpl>` (similar to `at::Tensor` that holds `c10::intrusive_ptr<c10::TensorImpl>`) 3. Most of `at::Generator*` usages were replaced with `at::Generator` TBD: replacing `Generator generator = nullptr` with `{}` requires JIT changes(adding Generator to IValue?) [ghstack-poisoned]

This PR adds c-tor `Generator(Device device)` to C++ API to match Python API, to avoid code duplication the logic was moved from `THPGenerator_pynew` to corresponding `at::Generator` c-tor. Unfortunately it makes Generator.h depend on CPUGenerator.h and CUDAGenerator.h, but this is how our Python API works :( * #34468 Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> [ghstack-poisoned]

…tr<GeneratorImpl>" This PR prepares `at::Generator` for pybind11's `type_caster<at::Generator>` which is required to implement custom RNG in python. The following changes are done: 1. `at::Generator` was moved to `c10::GeneratorImpl` (similar to `c10::TensorImpl`) 2. `at::Generator` was recreated as a holder of `std::shared_ptr<c10::GeneratorImpl>` (similar to `at::Tensor` that holds `c10::intrusive_ptr<c10::TensorImpl>`) 3. Most of `at::Generator*` usages were replaced with `at::Generator` TBD: replacing `Generator generator = nullptr` with `{}` requires JIT changes(adding Generator to IValue?) [ghstack-poisoned]

This PR adds c-tor `Generator(Device device)` to C++ API to match Python API, to avoid code duplication the logic was moved from `THPGenerator_pynew` to corresponding `at::Generator` c-tor. Unfortunately it makes Generator.h depend on CPUGenerator.h and CUDAGenerator.h, but this is how our Python API works :( * #34468 Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> [ghstack-poisoned]

…tr<GeneratorImpl>" This PR prepares `at::Generator` for pybind11's `type_caster<at::Generator>` which is required to implement custom RNG in python. The following changes are done: 1. `at::Generator` was moved to `c10::GeneratorImpl` (similar to `c10::TensorImpl`) 2. `at::Generator` was recreated as a holder of `std::shared_ptr<c10::GeneratorImpl>` (similar to `at::Tensor` that holds `c10::intrusive_ptr<c10::TensorImpl>`) 3. Most of `at::Generator*` usages were replaced with `at::Generator` TBD: replacing `Generator generator = nullptr` with `{}` requires JIT changes(adding Generator to IValue?) [ghstack-poisoned]

This PR adds c-tor `Generator(Device device)` to C++ API to match Python API, to avoid code duplication the logic was moved from `THPGenerator_pynew` to corresponding `at::Generator` c-tor. Unfortunately it makes Generator.h depend on CPUGenerator.h and CUDAGenerator.h, but this is how our Python API works :( * #34468 Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> [ghstack-poisoned]

ezyang · 2020-03-19T20:16:08Z

aten/src/ATen/Context.h

-        std::lock_guard<std::mutex> lock(cuda_gen.mutex_);
-        cuda_gen.set_current_seed(seed);
+        std::lock_guard<std::mutex> lock(cuda_gen->mutex_);
+        cuda_gen->set_current_seed(seed);


Also doesn't have to be this PR, but we should consider adding methods like set_current_seed to Generator directly, so dot-syntax works on them (making Generator consistent with Tensor)

will do in #34987

ezyang · 2020-03-19T20:16:44Z

aten/src/ATen/Utils.h

-    return static_cast<T*>(expr);
+static inline T* get_generator_or_default(const Generator& expr, const Generator& defaultValue) {
+  T* result = expr.defined() ? check_generator<T>(expr) : check_generator<T>(defaultValue);
+  if (result == nullptr) {


When can this be nullptr?

I rewrote this part, please take a look

ezyang · 2020-03-19T20:18:24Z

aten/src/ATen/core/Generator.h

-  virtual ~Generator() = default;
-  std::shared_ptr<Generator> clone() const;
+  // TODO(pbelevich): delete this after replace Generator generator = nullptr with c10::optional<at::Generator> = c10::nullopt
+  Generator(std::nullptr_t gen_impl) {}


It's pretty important for us to get rid of this TODO soon, as it's pretty confusing that:

std::shared_ptr<c10::GeneratorImpl> impl = nullptr; Generator g(impl); // errors Generator g(nullptr); // this is OK?!

Totally agree, #35067

ezyang · 2020-03-19T20:19:12Z

aten/src/ATen/core/Generator.h

-  virtual uint64_t seed() = 0;
-  Device device() const;
+  bool operator==(const Generator& rhs) const {
+    return (!(this->impl_) && !(rhs.impl_)) || (this->impl_ == rhs.impl_);


Can't this just be this->impl_ == rhs->impl_? Because if they're both nullptr they'll compare equal.

ezyang · 2020-03-19T20:21:48Z

aten/src/ATen/core/Generator.h


-  DispatchKeySet key_set() const { return key_set_; }
+  bool defined() const {
+    return (bool)impl_;


nit: static_cast<bool>(impl_) for extra safety. (Or just let implicit conversion on pointer take care of it.)

ezyang

LGTM.

ezyang · 2020-03-19T20:25:16Z

Please take care of replacing the = {} with explicit destructor call before landing

…tr<GeneratorImpl>" This PR prepares `at::Generator` for pybind11's `type_caster<at::Generator>` which is required to implement custom RNG in python. The following changes are done: 1. `at::Generator` was moved to `c10::GeneratorImpl` (similar to `c10::TensorImpl`) 2. `at::Generator` was recreated as a holder of `std::shared_ptr<c10::GeneratorImpl>` (similar to `at::Tensor` that holds `c10::intrusive_ptr<c10::TensorImpl>`) 3. Most of `at::Generator*` usages were replaced with `at::Generator` TBD: replacing `Generator generator = nullptr` with `{}` requires JIT changes(adding Generator to IValue?) [ghstack-poisoned]

…tr<GeneratorImpl>" This PR prepares `at::Generator` for pybind11's `type_caster<at::Generator>` which is required to implement custom RNG in python. The following changes are done: 1. `at::Generator` was moved to `c10::GeneratorImpl` (similar to `c10::TensorImpl`) 2. `at::Generator` was recreated as a holder of `std::shared_ptr<c10::GeneratorImpl>` (similar to `at::Tensor` that holds `c10::intrusive_ptr<c10::TensorImpl>`) 3. Most of `at::Generator*` usages were replaced with `at::Generator` TBD: replacing `Generator generator = nullptr` with `{}` requires JIT changes(adding Generator to IValue?) Differential Revision: [D20549420](https://our.internmc.facebook.com/intern/diff/D20549420) [ghstack-poisoned]

facebook-github-bot · 2020-03-22T02:08:53Z

@pbelevich merged this pull request in 5306713.

GeneratorHolder

51e79dd

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 9, 2020

GeneratorHolder

7c4dc1a

ghstack-source-id: dca28dd Pull Request resolved: #34468

pbelevich changed the title ~~GeneratorHolder~~ [WIP] GeneratorHolder Mar 9, 2020

Update on "[WIP] GeneratorHolder"

a188647

[ghstack-poisoned]

Update on "[WIP] GeneratorHolder"

9b9801f

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 9, 2020

GeneratorHolder

52023ae

ghstack-source-id: 1a8841c Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

65aada5

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 10, 2020

GeneratorHolder

9245402

ghstack-source-id: 5755613 Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

e31f545

[ghstack-poisoned]

pbelevich requested a review from apaszke as a code owner March 10, 2020 21:42

Update on "[WIP] GeneratorHolder"

c2f90ba

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 10, 2020

GeneratorHolder

e0e0031

ghstack-source-id: 66906e6 Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

3da834b

[ghstack-poisoned]

Update on "[WIP] GeneratorHolder"

5fa04a3

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 11, 2020

GeneratorHolder

5cf070d

ghstack-source-id: cbe8580 Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

816761d

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 12, 2020

GeneratorHolder

5e69e74

ghstack-source-id: 1108dee Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

d1e9990

[ghstack-poisoned]

Update on "[WIP] GeneratorHolder"

1ae46c3

[ghstack-poisoned]

Update on "[WIP] GeneratorHolder"

4e9b298

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 12, 2020

GeneratorHolder

ca5e0f1

ghstack-source-id: 6021401 Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

85a133c

[ghstack-poisoned]

pbelevich added a commit that referenced this pull request Mar 13, 2020

GeneratorHolder

730545e

ghstack-source-id: 5b5418a Pull Request resolved: #34468

Update on "[WIP] GeneratorHolder"

caf71b0

[ghstack-poisoned]

pbelevich mentioned this pull request Mar 18, 2020

Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> pytorch/xla#1778

Merged

pbelevich requested a review from ezyang March 19, 2020 20:08

ezyang reviewed Mar 19, 2020

View reviewed changes

ezyang approved these changes Mar 19, 2020

View reviewed changes

pbelevich mentioned this pull request Mar 19, 2020

Replace Generator with optional<Generator> instead of nullptr or Generator() #35067

Closed

pbelevich added 5 commits March 19, 2020 17:20

pbelevich mentioned this pull request Mar 21, 2020

Move normal() to DistributionTemplates #35167

Closed

facebook-github-bot closed this in 5306713 Mar 22, 2020

facebook-github-bot added the merged label Mar 22, 2020

facebook-github-bot deleted the gh/pbelevich/87/head branch March 25, 2020 14:18

junjihashimoto mentioned this pull request Jul 30, 2020

Update libtorch to 1.6 hasktorch/hasktorch#433

Merged

mruberry added the Merged label Oct 28, 2020

Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> #34468

Replace Generator* with Generator that holds std::shared_ptr<GeneratorImpl> #34468

Uh oh!

Conversation

pbelevich commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_xla_linux_xenial_py3_6_clang7_build (1/1)

🚧 1 upstream failure:

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!

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!

ezyang left a comment • 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!

ezyang commented Mar 19, 2020

Uh oh!

facebook-github-bot commented Mar 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pbelevich commented Mar 9, 2020 •

edited

Loading

dr-ci bot commented Mar 9, 2020 •

edited

Loading

ezyang left a comment •

edited

Loading