[JIT] add sorted keyword for lists and dicts #23274

eellison · 2019-07-23T22:15:33Z

Add sorted keyword to JIT for lists and dicts. This desugars to a list copy and a call to list.sort(). Since we don't have interfaces yet I implement it in terms of list.sort(). When we do we can re-visit implementing this op in a different manner.

The test fails bc of a fix to specialized lists which is landing here: #23267

Ignore the first commit because it is formatting, plz use clang_format ppl :'(

driazati

I think these should be implemented in operators instead of adding another SugaredValue, or we should add some mechanism to make composing operators together easier. The ad-hoc schema matching here isn't really a sustainable pattern and this kind of this will probably come up again in the future

driazati · 2019-07-24T00:27:29Z

torch/csrc/jit/script/sugared_value.cpp

+
+  if (inputs.size() != 1)
+    throw ErrorReport(loc)
+        << "sorted currently only accept any keyword arguments";


"only accepts 1 positional argument"

i didn't want to repeat all the code & ad-hoc schema checking that exists for aten::sort. i think it is more maintanable to compose this in terms of aten::sort

eellison · 2019-07-24T00:48:08Z

I think these should be implemented in operators instead of adding another SugaredValue, or we should add some mechanism to make composing operators together easier. The ad-hoc schema matching here isn't really a sustainable pattern and this kind of this will probably come up again in the future

I agree this isn't the most desirable end state, however this is a very special case of a builtin keyword. I don't think there are any other higher-order builtin functions in python. We've had a number of users internally & externally request sorted and this is a minimal implementation that enables it. I think we should revisit this in the future; but I don't think this should be blocked until who-knows-when.

eellison · 2019-07-24T00:54:49Z

@pytorchbot rebase this please

wanchaol · 2019-07-24T17:53:05Z

torch/csrc/jit/script/sugared_value.cpp

@@ -29,6 +29,42 @@ std::shared_ptr<SugaredValue> PrintValue::call(
  return std::make_shared<NoneValue>();
 }

+std::shared_ptr<SugaredValue> SortedValue::call(


I think most of this can be implemented in operators as @driazati suggested, you can just modularized/refactor the functions in register_prim_ops and implement the same thing for sorted as a builtinFunction.

if it were implemented in operators i would have to re-implement all of the schema checking within register_prim_ops that exists for aten::sort here which isn't very maintainable and should be avoided.

eellison · 2019-07-24T19:33:58Z

I also implemented it this way because I would like to add support for iterables, which isn't possible if it's not a sugared value. Or if it is i'm not sure how.

eellison · 2019-07-25T21:18:05Z

@pytorchbot rebase this please

…rs we support iterables

…ed_jit

eellison · 2019-07-26T19:46:15Z

@driazati @wanchaol i re-implemented it as a builtin operator. while on some level it would be nice to support dict, i think it would give user confusing belief that we support iterable arguments. since i'm no longer supporting dict (or plan to support iterables specifically through this operator) i did it as a builtin.

driazati

I don't see why we should drop dict support for this and keep list support since they're both actual values. Most users understand iterables to be a more advanced feature, but if we're doing lists we should do dicts as well.

driazati · 2019-07-26T19:59:06Z

torch/csrc/jit/register_prim_ops.cpp

+int listCopyAndSort(Stack& stack) {
+  c10::List<T> list = pop(stack).to<c10::List<T>>();
+  auto list_copied = list.copy();
+  std::sort(list_copied.begin(), list_copied.end(), [](const T& a, const T& b) {


Is the closure really necessary if it's just doing a < b?

driazati · 2019-07-26T19:59:44Z

torch/csrc/jit/register_prim_ops.cpp

+}
+
+template <typename T>
+int listCopyAndSort(Stack& stack) {


Naming these something like listSort and listSort_ would be more in line with existing torch naming on in place / out of place ops

This is a copy and a mutation, which isn't the same thing as an in place op

driazati · 2019-07-26T20:02:12Z

torch/csrc/jit/register_prim_ops.cpp

+    bool has_reverse_arg,
+    bool copy_return_list) {
+  return [lt_func, has_reverse_arg, copy_return_list](Stack& stack) {
+    bool reverse = has_reverse_arg ? pop(stack).toBool() : false;


The list of bool args for sort_op is confusing, this could be handled when the actual op is returned (i.e. here) instead of here which would clean things up.

i'm not sure what you mean

driazati · 2019-07-26T20:03:37Z

torch/csrc/jit/register_prim_ops.cpp

        listSort<bool>,
        aliasAnalysisFromSchema()),
+    Operator(
+        "aten::sorted(int[](a) input) -> (int[])",


The difference between sort and sorted isn't very obvious, how about aten::sort and aten::sort_ instead?

This is a mapping to a python builtin. It would also be confusing because you would assume aten::sort_ mutates the input, whereas sorted copies the input and sorts it

eellison · 2019-07-26T20:41:37Z

I don't see why we should drop dict support for this and keep list support since they're both actual values. Most users understand iterables to be a more advanced feature, but if we're doing lists we should do dicts as well.

The reason sorted(dict) works is because it calls iter on dict. There are a number of places where we have list as a type where the real type is iter, such as list(). It would be an improvement to be able to use iters here but i'd rather do that as a follow up / when we consider iterables more generally.

facebook-github-bot

@eellison is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-07-27T01:11:37Z

@eellison merged this pull request in 3497891.

Summary: Add `sorted` keyword to JIT for lists and dicts. This desugars to a list copy and a call to `list.sort()`. Since we don't have interfaces yet I implement it in terms of `list.sort()`. When we do we can re-visit implementing this op in a different manner. The test fails bc of a fix to specialized lists which is landing here: pytorch/pytorch#23267 Ignore the first commit because it is formatting, plz use clang_format ppl :'( Pull Request resolved: pytorch/pytorch#23274 Differential Revision: D16527323 Pulled By: eellison fbshipit-source-id: aed8faef23cb790b9af036cd6c1b9b1d7066345d

eellison added 2 commits July 23, 2019 15:07

format

78d21e7

add builtin sorted keyword

402cca2

eellison requested review from jamesr66a, driazati and wanchaol July 23, 2019 22:15

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: internals Related to internal abstractions in c10 and ATen labels Jul 23, 2019

driazati reviewed Jul 24, 2019

View reviewed changes

eellison requested a review from suo July 24, 2019 17:32

wanchaol reviewed Jul 24, 2019

View reviewed changes

driazati approved these changes Jul 24, 2019

View reviewed changes

wanchaol approved these changes Jul 24, 2019

View reviewed changes

eellison added 2 commits July 25, 2019 16:04

remove dict matching because it gives inconsistent expectation to use…

ac23354

…rs we support iterables

Merge branch 'master' of https://github.com/pytorch/pytorch into sort…

a8171ff

…ed_jit

eellison force-pushed the sorted_jit branch from a5b5b94 to a8171ff Compare July 25, 2019 23:04

new impl

bb4ed96

pytorchbot added the module: pybind Related to our Python bindings / interactions with other Python libraries label Jul 26, 2019

accidental changes

74cc9c3

eellison requested a review from driazati July 26, 2019 19:40

driazati reviewed Jul 26, 2019

View reviewed changes

driazati approved these changes Jul 26, 2019

View reviewed changes

facebook-github-bot reviewed Jul 26, 2019

View reviewed changes

facebook-github-bot closed this in 3497891 Jul 27, 2019

facebook-github-bot added the merged label Jul 27, 2019

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] add sorted keyword for lists and dicts #23274

[JIT] add sorted keyword for lists and dicts #23274

eellison commented Jul 23, 2019

driazati left a comment

driazati Jul 24, 2019

eellison Jul 24, 2019

eellison commented Jul 24, 2019

eellison commented Jul 24, 2019

wanchaol Jul 24, 2019

eellison Jul 24, 2019

eellison commented Jul 24, 2019 •

edited

Loading

eellison commented Jul 25, 2019

eellison commented Jul 26, 2019

driazati left a comment

driazati Jul 26, 2019

driazati Jul 26, 2019

eellison Jul 26, 2019

driazati Jul 26, 2019

eellison Jul 26, 2019

driazati Jul 26, 2019

eellison Jul 26, 2019

eellison commented Jul 26, 2019 •

edited

Loading

facebook-github-bot left a comment

facebook-github-bot commented Jul 27, 2019

[JIT] add sorted keyword for lists and dicts #23274

[JIT] add sorted keyword for lists and dicts #23274

Conversation

eellison commented Jul 23, 2019

driazati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eellison commented Jul 24, 2019

eellison commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eellison commented Jul 24, 2019 • edited Loading

eellison commented Jul 25, 2019

eellison commented Jul 26, 2019

driazati left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eellison commented Jul 26, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 27, 2019

eellison commented Jul 24, 2019 •

edited

Loading

eellison commented Jul 26, 2019 •

edited

Loading