fix schema matching of tuples to vartype lists by eellison · Pull Request #25944 · pytorch/pytorch

eellison · 2019-09-10T20:21:03Z

In schema matching we allow a homogenous tuple to be matched to list arguments. This logic wasn't yet extended for vartype lists, causing stuff like len((1, 2, 3)) to fail.

Fix for #20500

suo · 2019-09-10T20:30:07Z

I would prefer if @zdevito reviewed this as he was working on changing schema matching stuff recently

eellison · 2019-09-11T20:53:36Z

@suo most of the PR is just test / mechanical changes

eellison · 2019-09-11T22:43:19Z

@pytorchbot rebase this please

eellison · 2019-09-11T22:46:55Z

torch/csrc/jit/script/schema_matching.cpp

https://github.com/pytorch/pytorch/blob/master/torch/csrc/utils/python_arg_parser.cpp#L490

Specifies that this functionality is only supposed to work for intlists, but looking at test failures it looks like this is also being used for broadcast_tensors

zdevito · 2019-09-12T02:42:01Z

We hit a particularly nasty bug in the same codepath that is going to take a week of work to sort out, so I do want to carefully review this. Any change to type promotion rules should always have high scrutiny.

zdevito

This looks correct. A couple cleanups:

Test if the allow_conversions flag is really required, I think it will get caught by the isSubtypeOf checks that happen after matching anyway and it bloats the footprint of this change substantially.
Potentially move the tests to python if possible.

zdevito · 2019-09-12T06:45:12Z

aten/src/ATen/core/type.cpp

A for loop would be a lot cleaner here. It would remove the weird nullptr propagation logic, and allow for the short circute and return of nullptr.

zdevito · 2019-09-12T06:45:38Z

aten/src/ATen/core/type.cpp

Why does this return optional but unifyTypes return TypePtr that might be null. This should be consistent with TypePtr.

unifyTypes returns an optional

zdevito · 2019-09-12T06:47:19Z

aten/src/ATen/core/jit_type.h

I don't think we need this bool because we still check subtyping relationships after matching type variables. Even if we always allow potential matches, it will still have the same behavior.

zdevito · 2019-09-12T06:50:07Z

test/cpp/jit/test_schema_matching.cpp

use the testing namespace. Otherwise someone can write torch.test_vartype in the C++ frontend and it will work.

Will use the testing namespace - however this op only gets defined when the test is run right ? i don't think torch.test_vartype would be defined

as it is written, yes it will only appear here.

zdevito · 2019-09-12T06:51:31Z

test/cpp/jit/test_schema_matching.cpp

Can we avoid using low-level APIs like this in tests, it adds work to any refactoring. Please use Module and its define methods and ways of running methods.

actually, why are these C++ tests at all? If this is just running code but using a custom op, then just define the testing op in register_builtin_ops and write python tests.

I wrote the tests in c++ so that the op only gets defined when the tests are run and we don't have unnecessary ops in in the runtime.

zdevito · 2019-09-12T06:53:21Z

torch/csrc/jit/script/schema_matching.cpp

It is not specific to intlist, the comment is wrong.

eellison

Good point about not needing the allow_conversions arg. Re: c++. I would rather not mix together test ops and real ones. previously I used c++ for test ops

pytorch/test/cpp/jit/test_misc.cpp

Line 907 in efc5306

"prim::test_none() -> int?",

eellison · 2019-09-13T17:12:44Z

aten/src/ATen/core/type.cpp

unifyTypes returns an optional

eellison · 2019-09-13T17:14:08Z

test/cpp/jit/test_schema_matching.cpp

I wrote the tests in c++ so that the op only gets defined when the tests are run and we don't have unnecessary ops in in the runtime.

eellison · 2019-09-13T17:14:46Z

test/cpp/jit/test_schema_matching.cpp

Will use the testing namespace - however this op only gets defined when the test is run right ? i don't think torch.test_vartype would be defined

zdevito

Looks good!

facebook-github-bot

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

Summary: In schema matching we allow a homogenous tuple to be matched to list arguments. This logic wasn't yet extended for vartype lists, causing stuff like `len((1, 2, 3))` to fail. Fix for pytorch/pytorch#20500 Pull Request resolved: pytorch/pytorch#25944 Differential Revision: D17431514 Pulled By: eellison fbshipit-source-id: 2ad98bab15eaa496471df651572735eb35183323

facebook-github-bot · 2019-09-17T23:41:08Z

@eellison merged this pull request in a8073f3.

facebook-github-bot

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

eellison · 2019-09-19T18:35:12Z

@pytorchbot rebase this please

facebook-github-bot

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

Summary: In schema matching we allow a homogenous tuple to be matched to list arguments. This logic wasn't yet extended for vartype lists, causing stuff like `len((1, 2, 3))` to fail. Fix for pytorch/pytorch#20500 Pull Request resolved: pytorch/pytorch#25944 Differential Revision: D17482510 Pulled By: eellison fbshipit-source-id: aa63318c27a01d965a7a7b68ce8bec638168dc26

fix schema matching of tuples to vartype lists

3bf17a0

eellison requested a review from suo September 10, 2019 20:21

eellison requested a review from apaszke as a code owner September 10, 2019 20:21

pytorchbot added 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 Sep 10, 2019

suo requested a review from zdevito September 10, 2019 20:30

minor fix

32cbcd7

eellison mentioned this pull request Sep 11, 2019

[WIP][skip ci]List vartype matching #25327

Closed

eellison commented Sep 11, 2019

View reviewed changes

zdevito reviewed Sep 12, 2019

View reviewed changes

suo removed their request for review September 13, 2019 07:11

eellison commented Sep 13, 2019

View reviewed changes

respond to comments

b3aa3af

eellison force-pushed the fix_tuple_listvartype_matching branch from 8597000 to b3aa3af Compare September 16, 2019 22:12

eellison requested review from zdevito and removed request for zdevito September 17, 2019 00:03

revert_varargs_changes

ce50b71

eellison requested a review from zdevito September 17, 2019 17:21

zdevito approved these changes Sep 17, 2019

View reviewed changes

fix exception in tryCreateList

b63cdd7

facebook-github-bot reviewed Sep 17, 2019

View reviewed changes

facebook-github-bot closed this in a8073f3 Sep 17, 2019

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

eellison reopened this Sep 19, 2019

dont rely on aten

565b5a8

facebook-github-bot reviewed Sep 19, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into HEAD

40f7317

facebook-github-bot reviewed Sep 19, 2019

View reviewed changes

driazati mentioned this pull request Sep 19, 2019

[jit] VarType list matching doesn't work #26505

Closed

facebook-github-bot closed this in 0f42881 Sep 19, 2019

mruberry added the Merged label Oct 28, 2020

Conversation

eellison commented Sep 10, 2019

Uh oh!

suo commented Sep 10, 2019

Uh oh!

eellison commented Sep 11, 2019

Uh oh!

eellison commented Sep 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito commented Sep 12, 2019

Uh oh!

zdevito left a comment

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!

eellison left a comment

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!

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

facebook-github-bot commented Sep 17, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

eellison commented Sep 19, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants