Use c10::List #21177

smessmer · 2019-05-30T22:37:53Z

Stack:
:black_circle: #21177 Use c10::List 💛

Integrate c10::ListPtr into IValue and the c10 dispatcher.
Streamline conversion to/from IValue. Before, we had IValue::to<> and kernel_functor.h had its own ivalue_to_arg_type and return_type_to_ivalue. They are now unified. Also, this means that nested types like Dicts of Lists of Optional of Dict of ... do work as expected now

Differential Revision: D15476433

Differential Revision: D15476434 Differential Version: 83701491

Differential Revision: D15476434 Differential Version: 83720045

Differential Revision: D15476434 Differential Version: 83738373

Differential Revision: D15476433 Differential Version: 83738374

smessmer · 2019-05-30T22:38:46Z

This is a re-export of #20871 because some technical error screwed up that PR

Differential Revision: D15476433 Differential Version: 83744687

zdevito

After seeing this patch, I am still skeptical of the value add compared to the noise this change is adding. So many places look non-idiomatic for C++. The changes to register_prim and register_special also suggest that users will have to write quite a bit differently as well.

aten/src/ATen/core/type.cpp

tools/jit/gen_jit_dispatch.py

torch/csrc/jit/fuser/executor.cpp

torch/csrc/jit/graph_executor.cpp

torch/csrc/jit/register_c10_ops.cpp

torch/csrc/jit/register_prim_ops.cpp

Differential Revision: D15476433 Differential Version: 83838059

Differential Revision: D15476433 Differential Version: 83843752

Differential Revision: D15476433 Differential Version: 83895551

Differential Revision: D15476433 Differential Version: 84099249

aten/src/ATen/core/op_registration/kernel_functor.h

aten/src/ATen/core/op_registration/kernel_functor_test.cpp

Differential Revision: D15476433 Differential Version: 84235823

Differential Revision: D15476433 Differential Version: 84245863

Differential Revision: D15476433 Differential Version: 84268424

Differential Revision: D15476433 Differential Version: 84490438

Differential Revision: D15476433 Differential Version: 84493782

Differential Revision: D15476433 Differential Version: 84500329

Differential Revision: D15476433 Differential Version: 84639097

Differential Revision: D15476433 Differential Version: 84648896

Differential Revision: D15476433 Differential Version: 84651711

Differential Revision: D15476433 Differential Version: 84662129

Differential Revision: D15476433 Differential Version: 84667367

Differential Revision: D15476433 Differential Version: 84683317

Differential Revision: D15476433 Differential Version: 84727785

Differential Revision: D15476433 Differential Version: 84752433

Differential Revision: D15476433 Differential Version: 84756618

Differential Revision: D15476433 Differential Version: 84773353

Differential Revision: D15476433 Differential Version: 84774084

Differential Revision: D15476433 Differential Version: 84774561

Differential Revision: D15476433 Differential Version: 84782475

Differential Revision: D15476433 Differential Version: 84786750

Differential Revision: D15476433 Differential Version: 84789886

facebook-github-bot · 2019-06-12T22:13:59Z

This pull request has been merged in b527e48.

Summary: Pull Request resolved: pytorch/pytorch#21177 - Integrate c10::ListPtr into IValue and the c10 dispatcher. - Streamline conversion to/from IValue. Before, we had IValue::to<> and kernel_functor.h had its own ivalue_to_arg_type and return_type_to_ivalue. They are now unified. Also, this means that nested types like Dicts of Lists of Optional of Dict of ... do work as expected now Differential Revision: D15476433 fbshipit-source-id: bde9df80df20091aa8e6ae17ba7e90abd149b954

resistor · 2019-06-12T23:17:09Z

torch/csrc/jit/register_prim_ops.cpp

-  for (const auto& b_element : b->elements()) {
-    ret.push_back(b_element);
+  for (T b_element : b) {
+    ret.push_back(std::move(b_element));


This is a functional change that I don't think is correct without checking that the list being moved from has only a single use. See feedback from David and Michael on #21690

This isn't moving from the list. It's copying from the list into b_element and then moving from b_element.

out of curiosity, why was this code changed from the original way it was written?

I'd have expected something like

for (const T& b_element : b) { ret.push_back(b_element); }

Some ListPtr<T> (e.g. ListPtr<string> or nested lists/dicts) store their elements not as T but as IValues. When iterating over them, they have to create instances of T on the fly. This means we're copying T.

If the loop is for (T e : list), then this instance is directly created as e and can be moved from there. If the loop is for (const T& e : list), then the instance is also created as e, but now we can't move from it, so we need a second copy.

Summary: I have some test code in there as well, along with a script "test_libtorch" to run it. You'll need to modify `test_libtorch` to point to where you have `pytorch` built. I currently require that `pybind11` is included as a subdirectory of the test, but added it to the `.gitignore` to make this reviewable. Currently, something like this works: ```cpp struct Foo { int x, y; Foo(): x(2), y(5){} Foo(int x_, int y_) : x(x_), y(y_) {} void display() { cout<<"x: "<<x<<' '<<"y: "<<y<<endl; } int64_t add(int64_t z) { return (x+y)*z; } }; static auto test = torch::jit::class_<Foo>("Foo") .def(torch::jit::init<int64_t, int64_t>()) .def("display", &Foo::display) .def("add", &Foo::add) .def("combine", &Foo::combine); ``` with ```py torch.jit.script def f(x): val = torch._C.Foo(5, 3) val.display() print(val.add(3)) ``` results in ``` x: 5 y: 3 24 ``` Current issues: - [x] The python class created by torchscript doesn't interactly properly with the surrounding code. ``` torch.jit.script def f(x): val = torch._C.Foo(5, 3) return val ``` - [x] Doesn't properly take in non-pointer classes. Can't define this function signature in cpp (We don't want to support this I believe). ```cpp void combine(Foo x) { ``` - [x] Has some issues with memory for blobs when constructing multiple objects (fix constant propagation pass to not treat capsules as the same object). ```py torch.jit.script def f(x): val = torch._C.Foo(5, 3) val2 = torch._C.Foo(100, 0) val.display() print(val.add(3)) ``` - [ ] Can't define multiple constructors (need to define overload string. Currently not possible since we don't support overloaded methods). - [x] `init` is a little bit different syntax than `pybind`. `.init<...>()` instead of `.def(py::init<>())` - [x] I couldn't figure out how to add some files into the build so they'd be copied to the `include/` directories, so I symlinked them manually. - [ ] Currently, the conversion from Python into Torchscript doesn't work. - [ ] Torchbind also currently requires Python/Pybind dependency. Fixing this would probably involve some kind of macro to bind into Python when possible. - [ ] We pass back into Python by value, currently. There's no way of passing by reference. - [x] Currently can only register one method with the same type signature. This is because we create a `static auto opRegistry`, and the function is templated on the type signature. Somewhat blocked on #21177. We currently use some structures that will be refactored by his PR (namely `return_type_to_ivalue` and `ivalue_to_arg_type`. Pull Request resolved: #21098 Differential Revision: D16634872 Pulled By: Chillee fbshipit-source-id: 1408bb89ea649c27d560df59e2cf9920467fe1de

Summary: I have some test code in there as well, along with a script "test_libtorch" to run it. You'll need to modify `test_libtorch` to point to where you have `pytorch` built. I currently require that `pybind11` is included as a subdirectory of the test, but added it to the `.gitignore` to make this reviewable. Currently, something like this works: ```cpp struct Foo { int x, y; Foo(): x(2), y(5){} Foo(int x_, int y_) : x(x_), y(y_) {} void display() { cout<<"x: "<<x<<' '<<"y: "<<y<<endl; } int64_t add(int64_t z) { return (x+y)*z; } }; static auto test = torch::jit::class_<Foo>("Foo") .def(torch::jit::init<int64_t, int64_t>()) .def("display", &Foo::display) .def("add", &Foo::add) .def("combine", &Foo::combine); ``` with ```py torch.jit.script def f(x): val = torch._C.Foo(5, 3) val.display() print(val.add(3)) ``` results in ``` x: 5 y: 3 24 ``` Current issues: - [x] The python class created by torchscript doesn't interactly properly with the surrounding code. ``` torch.jit.script def f(x): val = torch._C.Foo(5, 3) return val ``` - [x] Doesn't properly take in non-pointer classes. Can't define this function signature in cpp (We don't want to support this I believe). ```cpp void combine(Foo x) { ``` - [x] Has some issues with memory for blobs when constructing multiple objects (fix constant propagation pass to not treat capsules as the same object). ```py torch.jit.script def f(x): val = torch._C.Foo(5, 3) val2 = torch._C.Foo(100, 0) val.display() print(val.add(3)) ``` - [ ] Can't define multiple constructors (need to define overload string. Currently not possible since we don't support overloaded methods). - [x] `init` is a little bit different syntax than `pybind`. `.init<...>()` instead of `.def(py::init<>())` - [x] I couldn't figure out how to add some files into the build so they'd be copied to the `include/` directories, so I symlinked them manually. - [ ] Currently, the conversion from Python into Torchscript doesn't work. - [ ] Torchbind also currently requires Python/Pybind dependency. Fixing this would probably involve some kind of macro to bind into Python when possible. - [ ] We pass back into Python by value, currently. There's no way of passing by reference. - [x] Currently can only register one method with the same type signature. This is because we create a `static auto opRegistry`, and the function is templated on the type signature. Somewhat blocked on pytorch/pytorch#21177. We currently use some structures that will be refactored by his PR (namely `return_type_to_ivalue` and `ivalue_to_arg_type`. Pull Request resolved: pytorch/pytorch#21098 Differential Revision: D16634872 Pulled By: Chillee fbshipit-source-id: 1408bb89ea649c27d560df59e2cf9920467fe1de

smessmer added 4 commits May 30, 2019 12:18

V4: Initial commit

d2e3189

Differential Revision: D15476434 Differential Version: 83701491

V5: (no description)

8bb9cef

Differential Revision: D15476434 Differential Version: 83720045

V6: Merge with parent diff changes

4f71794

Differential Revision: D15476434 Differential Version: 83738373

V7: Initial commit

d642b2c

Differential Revision: D15476433 Differential Version: 83738374

pytorchbot added caffe2 oncall: jit Add this issue/PR to JIT oncall triage queue module: cpp Related to C++ API module: internals Related to internal abstractions in c10 and ATen labels May 30, 2019

This was referenced May 30, 2019

List #21164

Closed

Use c10::List #20871

Closed

smessmer requested review from zdevito and dzhulgakov May 30, 2019 22:38

V8: (no description)

d6e6507

Differential Revision: D15476433 Differential Version: 83744687

zdevito reviewed May 31, 2019

View reviewed changes

V9: Merge with parent diff changes

08d048f

Differential Revision: D15476433 Differential Version: 83838059

smessmer changed the base branch from export-D15476434 to master May 31, 2019 17:20

smessmer requested a review from zdevito May 31, 2019 17:22

V10: (no description)

f4fdb99

Differential Revision: D15476433 Differential Version: 83843752

smessmer mentioned this pull request May 31, 2019

Streamline conversion to/from IValue #21228

Closed

smessmer added 2 commits May 31, 2019 16:05

V11: (no description)

eaaa03a

Differential Revision: D15476433 Differential Version: 83895551

V12: Merge with parent diff changes

ca5863c

Differential Revision: D15476433 Differential Version: 84099249

smessmer mentioned this pull request Jun 4, 2019

Remove double indirection for storing lists and dicts #21317

Closed

dzhulgakov reviewed Jun 4, 2019

View reviewed changes

aten/src/ATen/core/op_registration/kernel_functor.h Outdated Show resolved Hide resolved

aten/src/ATen/core/op_registration/kernel_functor_test.cpp Outdated Show resolved Hide resolved

smessmer added 2 commits June 4, 2019 12:41

V13: (no description)

4bdde78

Differential Revision: D15476433 Differential Version: 84235823

V14: (no description)

e3da833

Differential Revision: D15476433 Differential Version: 84245863

smessmer requested review from ebetica, goldsborough and yf225 as code owners June 4, 2019 20:55

V15: (no description)

94233f4

Differential Revision: D15476433 Differential Version: 84268424

smessmer added 7 commits June 6, 2019 15:00

V20: (no description)

8c43e08

Differential Revision: D15476433 Differential Version: 84490438

V21: (no description)

2fff0f2

Differential Revision: D15476433 Differential Version: 84493782

V22: Merge with parent diff changes

47fe718

Differential Revision: D15476433 Differential Version: 84500329

V24: Merge with parent diff changes

7265886

Differential Revision: D15476433 Differential Version: 84639097

V25: (no description)

129e238

Differential Revision: D15476433 Differential Version: 84648896

V26: (no description)

465b865

Differential Revision: D15476433 Differential Version: 84651711

V27: (no description)

a3d1471

Differential Revision: D15476433 Differential Version: 84662129

Chillee mentioned this pull request Jun 10, 2019

[jit] Initial torchbind prototype #21098

Closed

10 tasks

smessmer added 11 commits June 10, 2019 15:48

V28: (no description)

1b86c45

Differential Revision: D15476433 Differential Version: 84667367

V29: (no description)

557c3c1

Differential Revision: D15476433 Differential Version: 84683317

V30: (no description)

8c68e87

Differential Revision: D15476433 Differential Version: 84727785

V31: (no description)

f0e5e3d

Differential Revision: D15476433 Differential Version: 84752433

V32: Merge with parent diff changes

a1d4644

Differential Revision: D15476433 Differential Version: 84756618

V33: (no description)

e701f71

Differential Revision: D15476433 Differential Version: 84773353

V34: (no description)

3fb3f64

Differential Revision: D15476433 Differential Version: 84774084

V35: Merge with parent diff changes

a7194aa

Differential Revision: D15476433 Differential Version: 84774561

V36: Merge with parent diff changes

59d752b

Differential Revision: D15476433 Differential Version: 84782475

V37: (no description)

120f17d

Differential Revision: D15476433 Differential Version: 84786750

V38: (no description)

fb640a3

Differential Revision: D15476433 Differential Version: 84789886

facebook-github-bot closed this in b527e48 Jun 12, 2019

facebook-github-bot added the merged label Jun 12, 2019

resistor reviewed Jun 12, 2019

View reviewed changes

ezyang deleted the export-D15476433 branch July 19, 2019 15:49

eellison mentioned this pull request Dec 9, 2019

ONNX Interpolate Add Scales Params #28324

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use c10::List #21177

Use c10::List #21177

smessmer commented May 30, 2019 •

edited

smessmer commented May 30, 2019

zdevito left a comment

facebook-github-bot commented Jun 12, 2019

resistor Jun 12, 2019

smessmer Jun 12, 2019

bwasti Jun 13, 2019

smessmer Jun 13, 2019

Use c10::List #21177

Use c10::List #21177

Conversation

smessmer commented May 30, 2019 • edited

smessmer commented May 30, 2019

zdevito left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 12, 2019

resistor Jun 12, 2019

Choose a reason for hiding this comment

smessmer Jun 12, 2019

Choose a reason for hiding this comment

bwasti Jun 13, 2019

Choose a reason for hiding this comment

smessmer Jun 13, 2019

Choose a reason for hiding this comment

smessmer commented May 30, 2019 •

edited