add list methods: copy,extend #17092

Krovatkin · 2019-02-14T04:34:27Z

This PR adds the following methods to python's list.

copy
extend

and tests

Krovatkin · 2019-02-14T04:41:34Z

driazati · 2019-02-14T06:51:51Z

Unfortunately looks like we have some conflicts, an OSS contributor made PRs a few days ago for clear (#17050), reverse (#17001), and pop (#17015)

Krovatkin · 2019-02-14T07:10:23Z

I'll close this one, then.

TheCodez · 2019-02-14T07:56:55Z

@Krovatkin you could reopen it with just extends and copy, those are still missing.

Krovatkin · 2019-02-14T17:18:02Z

@TheCodez ah, thanks for letting me know! Will do!

suo · 2019-02-14T19:14:01Z

torch/csrc/jit/register_prim_ops.cpp

@@ -1286,6 +1314,15 @@ RegisterOperators reg2({
          "aten::append( " decl_type "[](a!) self, " decl_type              \
          "(c) el) -> " decl_type "[](a!)",                                 \
          listAppend<Shared<c_type>, c_type::ElemType>),                    \
+      Operator(                                                             \
+          "aten::extend(" decl_type "[](a!) self, " decl_type               \
+          " [](b) other) -> ()",                                            \


The b annotation here is unnecessary, since you're not doing a write or creating an alias. So it's fine to leave it blank (which is sugar for "this is a read-only, unique value")

suo · 2019-02-14T19:17:45Z

torch/csrc/jit/register_prim_ops.cpp

+      Operator(                                                             \
+          "aten::copy(" decl_type                                           \
+          "[](a) self)"                                                     \
+          " -> " decl_type "[](b!)",                                        \


The ! annotation is only necessary to denote mutations to existing values. So you don't (b!) on the return type, since it's a new value returned by the op. The rules are this way so that pure ops (the common case) don't need annotations, e.g. (add(Tensor a, Tensor b) -> Tensor).

I know this is confusing! I am doing a writeup of all the aliasing/mutation semantics to help clarify

apaszke · 2019-02-14T19:37:28Z

test/test_jit.py

+        for l in [[], [torch.rand(2)], [torch.rand(2), torch.rand(2), torch.rand(2)]]:
+            for r in [[], [torch.rand(2)], [torch.rand(2), torch.rand(2), torch.rand(2)]]:
+                rl = l[:]  # side effects due to extend
+                self.assertEqual(extend_list(l, r), rl + r)


Can we also verify that rl has been mutated?

I don't believe, rl (it should be a copy of l) would change, but rl + r would give us the correct answer to compare against l that does change. Why do we want rl to be mutated?

actually, I could switch the order of args in assertEqual that we shouldn't need a copy.

@apaszke scratch that. JIT makes a copy of every list argument meaning we never modify l.

driazati · 2019-02-14T22:01:35Z

torch/csrc/jit/register_prim_ops.cpp

+    TList list;
+    pop(stack, list);
+
+    const auto& vec = list->elements();


Does actually perform a copy?

a = [1, 2, 3] b = a.copy()

Won't creating an IValue from a reference to a result in it b sharing the same underlying data?

It will indeed perform a shallow copy.

const auto& vec = list->elements(); auto out = vec;

auto out is deduced to be std::vector<decl(vec)::value_type>, which isn't a reference type, so vec gets copied via an assignment operator into out. I believe this is an IValue c-tor that gets invoked in our case: IValue::IValue(std::vector<int64_t> v) (or a similar one corresponding to a given element type). We could explicitly pass a const reference (i.e. const auto& vec) into the c-tor and we should still get a shallow copy. I do prefer explicit copies and let a compiler elide them (even lvalue ones), even though passing a const reference should (in theory) prevent us from going to the wrong c-tor even if IValue::IValue(std::vector<int64_t> &v) gets added.

driazati · 2019-02-14T22:02:32Z

torch/csrc/jit/register_prim_ops.cpp

@@ -17,6 +17,7 @@
 #include <ATen/core/thread_pool.h>
 #include <c10/util/SmallVector.h>

+#include <algorithm>


What's this header for?

Ah, I'll remove it. I needed it for std::reverse. Thank you!

driazati · 2019-02-14T22:04:05Z

torch/csrc/jit/register_prim_ops.cpp

@@ -1003,6 +1004,33 @@ Operation listAppend(const Node* node) {
  };
 }

+template <typename TList>
+Operation listExtend(const Node* node) {


Operator() can take the std::function<int(Stack& stack) directly and this doesn't need node, so you can declare the op directly (e.g. int listExtend(Stack& stack) { ... })

None of the list operations seems to need a node and they all use the same c-tor. Maybe we should stick to that c-tor in order to be consistent here and follow up with another PR to update all the list ops wholesale?

Alrightie, created a separate task to track this one.

remove reverse,pop,clear put back accidentally removed tests revert some extra changes address feedback; fix test cases address David's feedback

facebook-github-bot

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

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Feb 14, 2019

Krovatkin closed this Feb 14, 2019

Krovatkin reopened this Feb 14, 2019

Krovatkin changed the title ~~[WIP] add list methods:clear,copy,extend,reverse~~ [WIP] add list methods: copy,extend Feb 14, 2019

Krovatkin force-pushed the krovatkin/list_methods branch 2 times, most recently from df873cc to a62e894 Compare February 14, 2019 19:03

Krovatkin changed the title ~~[WIP] add list methods: copy,extend~~ add list methods: copy,extend Feb 14, 2019

suo requested changes Feb 14, 2019

View reviewed changes

apaszke reviewed Feb 14, 2019

View reviewed changes

driazati reviewed Feb 14, 2019

View reviewed changes

TheCodez mentioned this pull request Feb 15, 2019

[JIT] Additional list methods in script #16662

Closed

add list methods:clear,copy,extend,reverse

8f09d09

remove reverse,pop,clear put back accidentally removed tests revert some extra changes address feedback; fix test cases address David's feedback

Krovatkin force-pushed the krovatkin/list_methods branch from 8c0d7ff to 8f09d09 Compare February 19, 2019 17:30

suo approved these changes Feb 19, 2019

View reviewed changes

facebook-github-bot reviewed Feb 19, 2019

View reviewed changes

facebook-github-bot closed this in a91e056 Feb 20, 2019

ezyang added the merged label Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add list methods: copy,extend #17092

add list methods: copy,extend #17092

Krovatkin commented Feb 14, 2019 •

edited

Loading

Krovatkin commented Feb 14, 2019

driazati commented Feb 14, 2019

Krovatkin commented Feb 14, 2019

TheCodez commented Feb 14, 2019

Krovatkin commented Feb 14, 2019

suo Feb 14, 2019

Krovatkin Feb 14, 2019

Krovatkin Feb 14, 2019

suo Feb 14, 2019

Krovatkin Feb 14, 2019

Krovatkin Feb 14, 2019

apaszke Feb 14, 2019

Krovatkin Feb 14, 2019 •

edited

Loading

Krovatkin Feb 14, 2019 •

edited

Loading

Krovatkin Feb 14, 2019

driazati Feb 14, 2019

Krovatkin Feb 15, 2019 •

edited

Loading

driazati Feb 14, 2019

Krovatkin Feb 15, 2019 •

edited

Loading

driazati Feb 14, 2019

Krovatkin Feb 15, 2019 •

edited

Loading

Krovatkin Feb 15, 2019

facebook-github-bot left a comment

add list methods: copy,extend #17092

add list methods: copy,extend #17092

Conversation

Krovatkin commented Feb 14, 2019 • edited Loading

Krovatkin commented Feb 14, 2019

driazati commented Feb 14, 2019

Krovatkin commented Feb 14, 2019

TheCodez commented Feb 14, 2019

Krovatkin commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Krovatkin Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

Krovatkin commented Feb 14, 2019 •

edited

Loading

Krovatkin Feb 14, 2019 •

edited

Loading

Krovatkin Feb 14, 2019 •

edited

Loading

Krovatkin Feb 15, 2019 •

edited

Loading

Krovatkin Feb 15, 2019 •

edited

Loading

Krovatkin Feb 15, 2019 •

edited

Loading