Align native_functions.yaml func schema more with JIT signature schema by cpuhrsch · Pull Request #16111 · pytorch/pytorch

cpuhrsch · 2019-01-17T16:24:40Z

This PR applies a few minor modifications leading to 100s of additional matches

Modifications to native_functions.yaml

double to float
int64_t to int
IntList[\d*] to int[\d*]
{} to []
Tensor? x=[] to Tensor? x=None
TensorList to Tensor[]
1e-x to 1e-0x
Generator* x = nullptr to Generator? x = None
{.*} to [.*]

Overall this adds about 300 new matches and brings us to about 1/2 compliance of native_functions func with their JIT signature equivalent

While this is still a draft "tools/jit/gen_jit_dispatch.py" contains code to aid in finding close signatures

wanchaol · 2019-01-17T23:17:08Z

aten/src/ATen/native_parse.py

shouldn't "int[]" be together with temp_type_translations?

facebook-github-bot

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

gchanan · 2019-01-18T20:20:47Z

aten/src/ATen/native/README.md

there are still a bunch of references to old types in this file that should be updated, e.g. Generator*, double when referring to the non-C++ schema, etc.

also the initializer list stuff is just wrong now.

gchanan · 2019-01-18T20:21:55Z

aten/src/ATen/native/native_functions.yaml

you repeat matches_jit_signature.

gchanan · 2019-01-18T21:03:48Z

aten/src/ATen/native_parse.py

you mean default argument here?

gchanan · 2019-01-18T21:05:28Z

aten/src/ATen/native_parse.py

how does this actually work in JIT? Like, how does it know what Mean is in parsing?

It doesn't. It does the inverse of this, which is translating 'Reduction::Mean' to 'Mean'.

gchanan · 2019-01-18T21:08:30Z

aten/src/ATen/native_parse.py

I'm confused -- "Tensor? x=[]" isn't the legacy -- when I looked in native_functions.yaml now, Tensor? weight={} is used.

facebook-github-bot

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

facebook-github-bot

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

ezyang · 2019-01-23T16:50:57Z

aten/src/ATen/native_parse.py

-    elif re.match(r'{.*}', s):
-        return s
+    elif re.match(r'\[.*\]', s):
+        return "{" + s[1:-1] + "}"


This is not an actionable comment, but it's kind of irritating the temp translations have to be spattered all over the code. Good thing they're labeled [temp translations]

facebook-github-bot

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

gchanan · 2019-01-23T19:22:19Z

aten/src/ATen/native_parse.py

        return '{}'
-    elif re.match(r'{.*}', s):
-        return s
+    elif re.match(r'\[.*\]', s):


nit: it's not obvious what's a temp translation and what's not without looking at the diff here.

gchanan · 2019-01-23T19:22:28Z

aten/src/ATen/native_parse.py

        return 'c10::nullopt'
+    # The JIT signature schema uses Mean, but in particular C++ needs
+    # the legacy Reduction::Mean. So we'll continue emiting that until
+    # we change this at either a JIT schema or C++ level.


why is there no [temp translations] label here?

gchanan · 2019-01-23T19:30:35Z

aten/src/ATen/native_parse.py

        default = None

+        # Enables Generator? by translating to legacy Generator*. See [temp translations]
+        if t == 'Generator?':


I think this could be simpler if you wrote another function temp_default_translations(default, type) and put all the special logic that isn't in temp_type_translations there.

Taking out the translations should basically require no thinking -- just rip out the entry from either temp_default_translations(default, type) or temp_type_translations

…a (#16111) Summary: This PR applies a few minor modifications leading to 100s of additional matches Modifications to native_functions.yaml 1) double to float 2) int64_t to int 3) IntList[\d*] to int[\d*] 4) {} to [] 5) Tensor? x=[] to Tensor? x=None 6) TensorList to Tensor[] 7) 1e-x to 1e-0x 8) Generator* x = nullptr to Generator? x = None 9) `{.*}` to `[.*]` Overall this adds about 300 new matches and brings us to about 1/2 compliance of native_functions func with their JIT signature equivalent While this is still a draft "tools/jit/gen_jit_dispatch.py" contains code to aid in finding close signatures Pull Request resolved: pytorch/pytorch#16111 Reviewed By: ezyang Differential Revision: D13738123 Pulled By: cpuhrsch fbshipit-source-id: d1ec1e089bdb26ec155f6f31ccf768270acb76c7

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

cpuhrsch force-pushed the simplematches branch 4 times, most recently from 6298d9f to 224416f Compare January 17, 2019 21:43

wanchaol reviewed Jan 17, 2019

View reviewed changes

aten/src/ATen/native_parse.py Outdated

Copy link

Collaborator

wanchaol Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't "int[]" be together with temp_type_translations?

facebook-github-bot reviewed Jan 18, 2019

View reviewed changes

gchanan reviewed Jan 18, 2019

View reviewed changes

cpuhrsch force-pushed the simplematches branch from 15aba97 to 43eb7e7 Compare January 22, 2019 16:15

facebook-github-bot reviewed Jan 22, 2019

View reviewed changes

cpuhrsch changed the title ~~[DRAFT] Align native_functions.yaml func schema more with JIT signature schema~~ Align native_functions.yaml func schema more with JIT signature schema Jan 22, 2019

cpuhrsch added 15 commits January 22, 2019 14:08

more numbers

cff9cf1

less numbers

3feb057

Updated README for new schema

c8d9f50

small edit

cc71e35

simpler

315af06

more comments

40c6a5b

better comments

f927ce5

Strange Caffe2 build error

77a8546

small reorg

91ed7b5

more build weirdness

c041a66

Remove duplicates

1c8b2cf

more rebase

15904c8

more more rebase

d648625

docs and lists

1e1a4bf

more matches

acd1353

cpuhrsch force-pushed the simplematches branch from a68e2f5 to acd1353 Compare January 22, 2019 22:11

deal with rebase

9011f29

facebook-github-bot reviewed Jan 23, 2019

View reviewed changes

ezyang reviewed Jan 23, 2019

View reviewed changes

ezyang approved these changes Jan 23, 2019

View reviewed changes

facebook-github-bot reviewed Jan 23, 2019

View reviewed changes

gchanan reviewed Jan 23, 2019

View reviewed changes

gchanan approved these changes Jan 23, 2019

View reviewed changes

facebook-github-bot closed this in f2b1842 Jan 23, 2019

zasdfgbnm mentioned this pull request Jan 29, 2019

Add namedtuple return for min, median, mode, kthvalue, add test for namedtuple return API #16186

Closed

ezyang added the merged label Jun 25, 2019

Conversation

cpuhrsch commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot 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!

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!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cpuhrsch commented Jan 17, 2019 •

edited

Loading