Implement sort for list of tuples #43448

gmagogsfm · 2020-08-22T02:35:07Z

Implement tuple sort by traversing contained IValue types and generate a lambda function as comparator for sort.
Tuple, class objects can now arbitrarily nest within each other and still be sortable

Fixes #43219

dr-ci · 2020-08-22T02:58:57Z

💊 CI failures summary and remediations

As of commit 1f33e25 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

2/2 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

❄️ 2 failures tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_macos_10_13_py3_test (1/2)

Step: "Update Homebrew" (full log | diagnosis details | 🔁 rerun) ❄️

fatal: Could not read from remote repository.

++ [[ -z /usr/local ]] 
++ dir=/usr 
++ [[ -n '' ]] 
++ [[ -z /usr ]] 
++ dir= 
++ [[ -n '' ]] 
++ [[ -z '' ]] 
++ [[ -n '' ]] 
+ git fetch --depth=1 origin 
Connection to github.com closed by remote host.  
fatal: Could not read from remote repository. 
 
Please make sure you have the correct access rights 
and the repository exists.

pytorch_ios_11_2_1_x86_64_build (2/2)

Step: "Update Homebrew" (full log | diagnosis details | 🔁 rerun) ❄️

fatal: Could not read from remote repository.

Receiving objects:  98% (179/182) Receiving objects:  99% (181/182) Receiving objects: 100% (182/182) Receiving objects: 100% (182/182), 64.85 KiB | 10.81 MiB/s, done. 
Resolving deltas:  96% (90/93) Resolving deltas:  97% (91/93) Resolving deltas: 100% (93/93) Resolving deltas: 100% (93/93), completed with 85 local objects. 
From ssh://github.com/Homebrew/homebrew-cask-versions 
 + 15f6b44...eb6e147 master     -> origin/master  (forced update) 
+ git reset --hard origin/master 
HEAD is now at eb6e147 Update tap_migrations.json (#9633) 
+ for path in '$(find /usr/local/Homebrew -type d -name .git)' 
+ cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/.git/.. 
+ git fetch --depth=1 origin 
Connection to github.com closed by remote host.  
fatal: Could not read from remote repository. 
 
Please make sure you have the correct access rights 
and the repository exists.

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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 123 times.

facebook-github-bot

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

SplitInfinity

Do you have any insight about the performance implications of this change? The sorting functions you removed were templated and converted the entire List<IValue> into a List<T> before sorting. The IValue comparison functions you added check the type tags of the IValue instances and call IValue::toXXX() on them while sorting. I think this is an probably an improvement in terms of code size (less template instantiations), but I'm curious about the runtime.

aten/src/ATen/core/ivalue.cpp

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

SplitInfinity · 2020-08-26T19:27:41Z

aten/src/ATen/core/ivalue.cpp

I wonder if there is an opportunity for performance improvement by implementing this explicitly instead of in terms of == and <.

But this proposal comes at the cost of increasing code size. I don't have strong feelings either way, just something to think about.

Good point. Done.

aten/src/ATen/core/ivalue.cpp

gmagogsfm · 2020-08-27T01:54:38Z

Do you have any insight about the performance implications of this change? The sorting functions you removed were templated and converted the entire List<IValue> into a List<T> before sorting. The IValue comparison functions you added check the type tags of the IValue instances and call IValue::toXXX() on them while sorting. I think this is an probably an improvement in terms of code size (less template instantiations), but I'm curious about the runtime.

Good point, I changed basic types' sort kernel back to template-based. Tuple sorting is still dependent on runtime type refining and type checking, so the comparison logic of IValue is still retained.

facebook-github-bot

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

facebook-github-bot

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

eellison

Looks good, I think we need some small changes to the runtime op. Do we need to add the > operator ?

It's a little weird now how we allow sorting of basic types, tuples containing them, and also class types, but we don't allow sorting of tuples containing class types. The downside in the current approach with class types is you would have to look up the < function on each comparison, but i think that's probably fine ? I dont know how many models do much class sorting. If that is fine we should merge this implementation with the class impleemntation.

eellison · 2020-08-27T16:22:09Z

aten/src/ATen/core/ivalue.cpp

can we remove this ?

https://en.cppreference.com/w/cpp/algorithm/sort says std::sort only needs operator<, so yeah, maybe. 😮

As mentioned in another comment, std::greater<> needs it for reverse sorting.

eellison · 2020-08-27T20:05:50Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Can you rephrase this ? We can;t handle all tuples, just the tuples we can sort are only those comprised of the other types...

Yeah, maybe something like or Tuples of the aforementioned type.

Done, with the caveat that it isn't exactly true for tuple of classes. Let's discuss in the other comment thread.

eellison · 2020-08-27T20:06:58Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

we don't seem to be doing the reverse here

Oops, brainfarted. Really shouldn't have done this at midnight.

eellison · 2020-08-27T20:09:30Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

We're checking copy_return_list three times in this function, can your refactor this function? it's a little hard to follow

Done. Reduced to two times, each maps to one distinct action required.

eellison · 2020-08-27T20:29:16Z

aten/src/ATen/core/ivalue.cpp

Do we need to define the > operator ?

Yeah, I am using std::greater<>() to implement reverse sorting, it relies on operator> .

test/test_jit.py

gmagogsfm

Looks good, I think we need some small changes to the runtime op. Do we need to add the > operator ?

It's a little weird now how we allow sorting of basic types, tuples containing them, and also class types, but we don't allow sorting of tuples containing class types. The downside in the current approach with class types is you would have to look up the < function on each comparison, but i think that's probably fine ? I dont know how many models do much class sorting. If that is fine we should merge this implementation with the class impleemntation.

Right, tuple of classes can't be sorted due to how classes are handled (I assume that's what you mean by current approach?). I think the main benefit of looking up "<" function for each class is that objects of different class types can be compared against each other as long as they have lt implemented with right type. Do you mean this is an overhead that we should get rid of?

As for merging "this implementation with class implementation", do you mean we should unify every comparison logic into that of IValue? so they can arbitrarily nest?

test/test_jit.py

gmagogsfm · 2020-08-27T23:27:23Z

aten/src/ATen/core/ivalue.cpp

Yeah, I am using std::greater<>() to implement reverse sorting, it relies on operator> .

gmagogsfm · 2020-08-27T23:27:56Z

aten/src/ATen/core/ivalue.cpp

As mentioned in another comment, std::greater<> needs it for reverse sorting.

test/test_jit.py

gmagogsfm · 2020-08-27T23:30:10Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Done. Reduced to two times, each maps to one distinct action required.

gmagogsfm · 2020-08-27T23:30:28Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Oops, brainfarted. Really shouldn't have done this at midnight.

gmagogsfm · 2020-08-27T23:48:30Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Done, with the caveat that it isn't exactly true for tuple of classes. Let's discuss in the other comment thread.

codecov · 2020-09-04T13:03:30Z

Codecov Report

Merging #43448 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #43448   +/-   ##
=======================================
  Coverage   68.07%   68.07%           
=======================================
  Files         384      384           
  Lines       49765    49765           
=======================================
  Hits        33879    33879           
  Misses      15886    15886

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 993b465...1f33e25. Read the comment docs.

eellison · 2020-09-04T18:00:53Z

@gmagogsfm could you comment on the large changes ? Why did we have to add the getLessThanComparator ?

gmagogsfm · 2020-09-04T18:28:55Z

@gmagogsfm could you comment on the large changes ? Why did we have to add the getLessThanComparator ?

This new change is actually still WIP. Sorry for pushing it to gh early, I wanted CI to run. Will update comment once some more changes are done.

eellison · 2020-09-04T19:20:58Z

You're good, just trying to do my review requested queue. Removing my name for now you can re-request when its ready.

gmagogsfm · 2020-09-04T21:01:29Z

You're good, just trying to do my review requested queue. Removing my name for now you can re-request when its ready.

Just pushed another revision. The main reason behind this change to to make tuples sortable even if there are other types nested in it.

getIValueLessThanComparator is a one time cost to construct a comparator function specific to each tuple type. Combined with the fact that we enforce all tuple types to be exactly same at runtime check, we can use this comparator to compare every pair of tuples. Since getIValueLessThanComparator also supports classes, now class objects can nest inside tuples and be sortable.

Tests are still running but weekend is right ahead of us, so pushing it out to review now for some early comments.

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

aten/src/ATen/core/ivalue.cpp

SplitInfinity · 2020-09-04T21:40:52Z

aten/src/ATen/core/ivalue.cpp

SplitInfinity · 2020-09-04T21:48:28Z

test/test_jit.py

Consider adding a test case to test the error message for unsortability due to unsupported types or differing types in a nested tuple. e.g.

[(1, (2, {1: 2})), (1, (2, {1: 2})), (1, (0, {1: 2}))]

Done. Added a test for different types. Unsortable types is covered in an earlier test.

SplitInfinity

All my comments have been addressed, thanks. I will defer to @eellison for approval.

SplitInfinity · 2020-09-10T05:44:09Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Intentional?

yes, slightly clearer.

SplitInfinity · 2020-09-10T05:45:21Z

test/cpp/tensorexpr/test_reductions.cpp

SplitInfinity · 2020-09-10T05:46:01Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Intentional?

Yes, to fix a previous accident lol

eellison

This looks good, would be nice to refactor tuples and list sorting to reuse more logic

eellison · 2020-09-10T17:50:11Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

we're double checking copy_return_list here

Discussed offline, this is slightly clearer.

eellison · 2020-09-10T17:57:39Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Now that tuples & classes can compose, could we refactor this so we just have sortIValueListOrThrow or something along those lines ?

eellison · 2020-09-15T18:25:52Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

It's surprising to me that we have a separate isSortableTupleType - could we refactor this to isSortableType, and within the Tuple Case recurse on the type members?

eellison · 2020-09-15T18:26:56Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Similarly - this seems like a general restriction of list sorting, would be nice to refactor to remove tuple-specific casing

eellison · 2020-09-15T18:27:49Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Could we try to refactor this with isSortableListofTuples?

facebook-github-bot

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

eellison

LGTM! Thanks for making the changes. One small remaining nit

eellison · 2020-09-16T19:04:20Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Can we refactor isSortableTupleType to isSortableType and then just call that here instead of the two separate branches for Class / Tuple ? It would be a very small change - change input of isSortableTupleType to a single type instead of tuple_type and not iterate and it works

This is actually intentional with a very subtle reason.

If we use isSortableType which, by definition, returns true for other basic types (numbers, strings and tensors), that creates the hazard of someone mistakenly messed up schema matching of aten::sort.basic_types and we end up missing the chance of throwing an error here that clearly says type is incorrect because now we need to return true for them. Not a huge deal, but would like to know what you think on this.

facebook-github-bot

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

…bjects can be sorted,

facebook-github-bot

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

facebook-github-bot · 2020-09-17T20:23:24Z

@gmagogsfm merged this pull request in 2558e57.

Summary: * Implement tuple sort by traversing contained IValue types and generate a lambda function as comparator for sort. * Tuple, class objects can now arbitrarily nest within each other and still be sortable Fixes #43219 Pull Request resolved: #43448 Reviewed By: eellison Differential Revision: D23352273 Pulled By: gmagogsfm fbshipit-source-id: b6efa8d00e112178de8256da3deebdba7d06c0e1

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 22, 2020

gmagogsfm requested review from SplitInfinity, eellison and zdevito August 26, 2020 18:34

gmagogsfm marked this pull request as ready for review August 26, 2020 18:35

gmagogsfm requested a review from apaszke as a code owner August 26, 2020 18:35

facebook-github-bot reviewed Aug 26, 2020

View reviewed changes

SplitInfinity reviewed Aug 26, 2020

View reviewed changes

facebook-github-bot reviewed Aug 27, 2020

View reviewed changes

eellison reviewed Aug 27, 2020

View reviewed changes

SplitInfinity reviewed Aug 27, 2020

View reviewed changes

test/test_jit.py Outdated Show resolved Hide resolved

test/test_jit.py Outdated Show resolved Hide resolved

gmagogsfm commented Aug 27, 2020

View reviewed changes

gmagogsfm requested a review from eellison September 1, 2020 19:34

eellison removed their request for review September 4, 2020 19:21

gmagogsfm requested review from SplitInfinity and eellison September 4, 2020 21:08

SplitInfinity reviewed Sep 4, 2020

View reviewed changes

gmagogsfm requested review from SplitInfinity and wanchaol September 10, 2020 05:40

SplitInfinity reviewed Sep 10, 2020

View reviewed changes

eellison reviewed Sep 15, 2020

View reviewed changes

Implement tuple[] sort

6f7da1c

gmagogsfm requested a review from eellison September 15, 2020 23:44

facebook-github-bot reviewed Sep 16, 2020

View reviewed changes

eellison approved these changes Sep 16, 2020

View reviewed changes

facebook-github-bot reviewed Sep 16, 2020

View reviewed changes

Change method to generate sorting comparator so that tuple of class o…

1f33e25

…bjects can be sorted,

facebook-github-bot reviewed Sep 17, 2020

View reviewed changes

facebook-github-bot closed this in 2558e57 Sep 17, 2020

facebook-github-bot added the merged label Sep 17, 2020

mruberry added the Merged label Oct 28, 2020

Implement sort for list of tuples #43448

Implement sort for list of tuples #43448

Uh oh!

Conversation

gmagogsfm commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

❄️ 2 failures tentatively classified as flaky

pytorch_macos_10_13_py3_test (1/2)

pytorch_ios_11_2_1_x86_64_build (2/2)

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

SplitInfinity 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmagogsfm commented Aug 27, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

eellison 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmagogsfm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gmagogsfm commented Aug 22, 2020 •

edited

Loading

dr-ci bot commented Aug 22, 2020 •

edited

Loading

SplitInfinity left a comment •

edited

Loading

eellison left a comment •

edited

Loading

codecov bot commented Sep 4, 2020 •

edited

Loading