Remove (almost all) TensorOptions from native_functions.yaml #17385

cpuhrsch · 2019-02-22T01:26:50Z

Stacked on top of #17386

Brings us to 1014/1106 of writing.

facebook-github-bot

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

cpuhrsch · 2019-02-27T23:11:42Z

cpuhrsch · 2019-02-28T02:18:34Z

Note: It's debatable whether we want to add support for long for ScalarType to the JIT and also if we might want to call it int64_t or int instead of long.

cpuhrsch · 2019-02-28T02:19:10Z

tools/jit/gen_jit_dispatch.py

Will be removed.

cpuhrsch · 2019-02-28T02:19:18Z

aten/src/ATen/native_parse.py

Will be removed.

facebook-github-bot

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

cpuhrsch · 2019-02-28T06:22:03Z

tools/jit/gen_jit_dispatch.py

will be removed

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.

cpuhrsch · 2019-03-01T06:10:19Z

This is the changeset of the generated code.

gchanan · 2019-03-01T16:02:32Z

aten/src/ATen/native/native_functions.yaml

These defaults are wrong:
dtype: the default is the default tensor type
layout: strided is correct for some of these, but wrong for others (e.g. sparse_coo_tensor below has default strided which is clearly wrong)
device: the default device is the device of the default tensor type.

Is there a reason we can't make the defaults for all of these None if they are determined at runtime? (It's fine to have a real default for layout but it should be correct).

These defaults are determined based on what the JIT generates. They are indeed not logical, but that doesn't mean they're wrong in the way it's currently implemented.

Marking this for further edits: Shouldn't have defaults

gchanan · 2019-03-01T16:08:18Z

aten/src/ATen/native/native_functions.yaml

these types and defaults look not-pythonic. You don't have to change this now, but what's the long term plan?

i.e. ScalarType -> Dtype(?), in python this is "torch.dtype", but "dtype dtype" looks a bit strange. Maybe we should just write out the full types? e.g:

func: bartlett_window(int window_length, *, torch.dtype dtype=None, torch.layout layout=torch.strided, torch.device device=None) -> Tensor

Although then I guess you should write out torch.Tensor which seems annoying.

Thoughts? CC @zdevito.

Keep in mind, this also needs be used in the JIT.

gchanan · 2019-03-01T16:09:08Z

aten/src/ATen/native/native_functions.yaml

this look wrong -- doesn't this mean you have to provide values for dtype, layout, device?

Even more so, how can a keyword argument "options" not require a default value?

If you do provide this a default value, it would, logically, make sense to provide one for the generated C++ code, however, you'll then get conflicts with empty_like(Tensor self) and empty_like(Tensor self, TensorOptions options={}), because it's ambiguous which symbol to use.

ah, ok, the issue is this isn't combined with the above definition. Do you know why that is?

facebook-github-bot

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

cpuhrsch · 2019-03-01T23:36:24Z

$ git diff torch/csrc/jit/generated/register_aten_ops_* | gist-paste
https://gist.github.com/ba972f121fda1e6ccb5318a8af2ab343

facebook-github-bot

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

cpuhrsch · 2019-03-04T23:04:05Z

torch/csrc/jit/operator.cpp

Still needs work / can be removed?

zdevito · 2019-03-11T20:26:36Z

test/test_jit.py

yeah, this seems like an improvement -- previously we had to mess with the dtype to get this to work right.

Yes, but this appears to be restricted to tracing

gchanan · 2019-03-11T22:02:59Z

aten/src/ATen/native_parse.py

-        # device is specified as an IntArrayRef of { at::Device::Type, device_id }
-        {'name': 'device', 'type': ['Device', 'Device?'], 'is_nullable': False, 'annotation': None},
+    def copy_arguments(arguments):
+        new_arguments = []


copy.deepcopy?

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.

Summary: Stacked on top of pytorch/pytorch#17386 Brings us to 1014/1106 of writing. Pull Request resolved: pytorch/pytorch#17385 Differential Revision: D14248008 Pulled By: cpuhrsch fbshipit-source-id: 033e00de91e3edf7ae01ca03ebe436c0446b3b5c

* upstream/master: (87 commits) Make Variable::set_data non-const; cosmetic fixes. remove warning for upsample code (pytorch#17921) Optimize TileOp (pytorch#17290) Optimize channel_stats_op (pytorch#16243) enable shape inference for elementwise operators (pytorch#17885) Remove remaining test jit expects redux (pytorch#17924) Handle Scalars Better (pytorch#17875) Fixed a formatting issue in doc comments (pytorch#17505) Add nbytes, itemsize, element_size to at::Tensor. (pytorch#17810) Fix lint in test_distributions.py Fix lint in test_jit.py Fix lint errors in test_autograd Added a few extra python bindings to help with walking the IR graph from Python (pytorch#17822) kthvalue consistency with sort in the presence of NaN (pytorch#17824) Fix minor grammatical mistakes in torch/nn/modules/loss.py (pytorch#17892) Remove (almost all) TensorOptions from native_functions.yaml (pytorch#17385) Restore full Windows tests (pytorch#17102) Prevent VS2017 from emitting ambiguous symbol errors (second time) Fix windows test hang (pytorch#17778) torch.btrifact for tensors with greater than 3 dimensions (pytorch#14964) ...

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

cpuhrsch force-pushed the ttsl branch 3 times, most recently from cc63fd9 to 325fd73 Compare February 27, 2019 02:13

facebook-github-bot reviewed Feb 27, 2019

View reviewed changes

cpuhrsch force-pushed the ttsl branch from 3828d5a to 2a730ae Compare February 27, 2019 23:10

cpuhrsch force-pushed the ttsl branch from 58980e2 to c55ae8a Compare February 28, 2019 02:14

cpuhrsch commented Feb 28, 2019

View reviewed changes

tools/jit/gen_jit_dispatch.py Outdated

Copy link

Contributor Author

cpuhrsch Feb 28, 2019

Choose a reason for hiding this comment

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

Will be removed.

cpuhrsch commented Feb 28, 2019

View reviewed changes

aten/src/ATen/native_parse.py Outdated

Copy link

Contributor Author

cpuhrsch Feb 28, 2019

Choose a reason for hiding this comment

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

Will be removed.

facebook-github-bot reviewed Feb 28, 2019

View reviewed changes

cpuhrsch commented Feb 28, 2019

View reviewed changes

tools/jit/gen_jit_dispatch.py Outdated

Copy link

Contributor Author

cpuhrsch Feb 28, 2019

Choose a reason for hiding this comment

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

will be removed

cpuhrsch force-pushed the ttsl branch 2 times, most recently from b1680cc to 26e6924 Compare February 28, 2019 21:46

facebook-github-bot reviewed Feb 28, 2019

View reviewed changes

facebook-github-bot reviewed Mar 1, 2019

View reviewed changes

cpuhrsch changed the title ~~[DRAFT] Remove TensorOptions from native_functions.yaml~~ Remove TensorOptions from native_functions.yaml Mar 1, 2019

gchanan reviewed Mar 1, 2019

View reviewed changes

cpuhrsch force-pushed the ttsl branch from b437cb7 to 0a68719 Compare March 1, 2019 22:43

cpuhrsch changed the title ~~Remove TensorOptions from native_functions.yaml~~ Remove (almost all) TensorOptions from native_functions.yaml Mar 1, 2019

facebook-github-bot reviewed Mar 1, 2019

View reviewed changes

cpuhrsch force-pushed the ttsl branch 2 times, most recently from 2d6021f to 8bfda98 Compare March 2, 2019 01:21

facebook-github-bot reviewed Mar 2, 2019

View reviewed changes

cpuhrsch force-pushed the ttsl branch 2 times, most recently from 7c7af73 to c850bc0 Compare March 4, 2019 18:12

cpuhrsch commented Mar 4, 2019

View reviewed changes

torch/csrc/jit/operator.cpp Outdated

Copy link

Contributor Author

cpuhrsch Mar 4, 2019

Choose a reason for hiding this comment

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

Still needs work / can be removed?

cpuhrsch added 15 commits March 11, 2019 11:53

bring back long

9f2c6e1

more changes to nativ schema

338acc1

more matches

703ece6

more more matches

0ef26c9

deal with None

c2174db

remove debug messages

d867ef3

optional

2b831d6

hardcoded ops

1994103

deal with none

b795183

deal with shapes

dc109db

adopt test

2f3e550

update tests

06ca723

remove comments

e377780

edit comment

f08927c

much better variable name

8b5e73c

cpuhrsch requested a review from zdevito March 11, 2019 19:27

zdevito reviewed Mar 11, 2019

View reviewed changes

clean up parse

78842e9

cpuhrsch force-pushed the ttsl branch from 1128810 to 78842e9 Compare March 11, 2019 21:02

improved warning

b37278a

gchanan approved these changes Mar 11, 2019

View reviewed changes

facebook-github-bot reviewed Mar 11, 2019

View reviewed changes

use deepcopy

0588a80

facebook-github-bot reviewed Mar 11, 2019

View reviewed changes

Krovatkin mentioned this pull request Mar 12, 2019

torch.arange dtype mismatch with and without jit #17662

Closed

facebook-github-bot closed this in 02c48cc Mar 12, 2019

pytorchbot added the merged label Mar 12, 2019

Remove (almost all) TensorOptions from native_functions.yaml #17385

Remove (almost all) TensorOptions from native_functions.yaml #17385

Uh oh!

Conversation

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

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

cpuhrsch commented Feb 27, 2019

Uh oh!

cpuhrsch commented Feb 28, 2019

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!

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!

cpuhrsch commented Mar 1, 2019

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!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

cpuhrsch commented Mar 1, 2019

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cpuhrsch commented Feb 22, 2019 •

edited

Loading