Optional ScalarType support for native functions & JIT by tugrulates · Pull Request #15154 · pytorch/pytorch

tugrulates · 2018-12-13T03:33:28Z

This completes the support for optional in native, JIT and autograd.

Note: Mostly following the existing implementation for optional that was added in #12582.

This PR introduces a way to make functions accept an optional dtype and it will unblock #9515 by allowing the dtype param for type promotion interface:

func: name(inputs, *, ScalarType? dtype=None, Casting casting=same_kind)

An alternative approach could have been using ScalarType::Undefined for the same purpose but without optional, though it would have been a bit hacky.

func: name(inputs, *, ScalarType dtype=Undefined, Casting casting=same_kind)

Here's an example use of this in action: 971f69e

There are already a bunch of native functions that were getting optional dtype through function overloading. #15133 is the attempt to migrate all of those. I will send those changes separately after this since some functions (e.g. sum) need quite a bit of change in the codebase. See the commits over there.

tugrulates · 2018-12-13T03:35:32Z

torch/csrc/jit/passes/to_batch.cpp

    else if(n->kind().is_prim()){
      switch(n->kind()){
        case prim::Constant:
+        case prim::None:


This just tells to copy None values in JIT graph to batch JIT graph as is.

tugrulates · 2018-12-13T03:35:53Z

torch/csrc/utils/python_arg_parser.cpp

  {"Storage", ParameterType::STORAGE},
  {"PyObject*", ParameterType::PYOBJECT},
  {"ScalarType", ParameterType::SCALARTYPE},
-  {"optional<ScalarType>", ParameterType::SCALARTYPE},


This wasn't used and not needed.

gchanan · 2018-12-13T19:51:04Z

tools/autograd/gen_python_functions.py

        if arg.get('default') is not None:
            default = arg['default']
-            if default == 'nullptr' or default == 'nullopt' or default == '{}':
+            if default == 'nullptr' or default == 'nullopt' or default == 'c10::nullopt' or default == '{}':


why do you need c10::nullopt? where does that come from?

I remember JIT was generating that (register_aten_ops) last week. It doesn't seem to be needed any more, so removed this.

Ah, this was still needed. It's added in

pytorch/aten/src/ATen/native_parse.py

Line 26 in b6edd7b

return 'c10::nullopt'

I will either revert this or find a different fix in the next PR.

facebook-github-bot

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

zou3519 · 2018-12-14T03:24:02Z

How hard is it to just use optional<ScalarType> instead of ScalarType??

zou3519

Is there an easy way to add a test?

I don't know enough about this part of the JIT to comment on the correctness but it looks very similar to the original PR that added optional type. Maybe @wanchaol or @zdevito can take a look here?

wanchaol

Thank for adding this! the PR LGTM. Can you add a test in JIT to test a op that takes Optional[ScalarType], pass a None into it and see if it works end to end?

zasdfgbnm · 2018-12-14T21:06:43Z

Could we add test for this by creating a dummy aten ops on native_functions.yaml to use these features, and test it on test_jit.py by using these ops? Sounds ugly, but easy to implement. I'm adding support for optional<int64_t> in #15235 and looking for a good way to test my code too.

tugrulates · 2018-12-14T21:22:08Z

I'm about to convert ? suffix in type system to optional. Will add a test for optional<Scalar> in test_jit soon here.

tugrulates · 2018-12-14T23:40:08Z

Added another commit which converts all optional types except Tensor? from T? to optional<T>. cc @gchanan

Why leave Tensor out? Tensor? accepts undefined Tensors. There's a bunch of TODOs in the codebase to clean this up. I added a few more TODOs into locations that needs cleanup too.

Why not wait for Tensor? to be cleaned up? More and more needs for optional types and it's becoming harder to clean in ? suffix format.

#12582 added optional support by following Tensor? syntax. This PR now changes the syntax to use optional<>. @wanchaol @zdevito was there any discussion around this around that time? Any specific reason the optional<> syntax can horribly fail?

In any case, I can still drop the last commit and use ScalarType? for the original need in this PR.

tugrulates · 2018-12-14T23:54:27Z

Existing JIT tests that use optional<Device> and optional<Scalar> should cover this end to end. I updated the expect files wherever needed. PTAL

There is currently no op using optional<ScalarType>. I have a stack of conversion here:
https://github.com/tugrulates/pytorch/commits/optional-scalartype

I'm holding off on sending them before this PR is resolved. I plan to add a JIT test for optional<ScalarType> with the first op that uses it.

wanchaol · 2018-12-14T23:55:25Z

Added another commit which converts all optional types except Tensor? from T? to optional<T>. cc @gchanan

Why leave Tensor out? Tensor? accepts undefined Tensors. There's a bunch of TODOs in the codebase to clean this up. I added a few more TODOs into locations that needs cleanup too.

Why not wait for Tensor? to be cleaned up? More and more needs for optional types and it's becoming harder to clean in ? suffix format.

#12582 added optional support by following Tensor? syntax. This PR now changes the syntax to use optional<>. @wanchaol @zdevito was there any discussion around this around that time? Any specific reason the optional<> syntax can horribly fail?

In any case, I can still drop the last commit and use ScalarType? for the original need in this PR.

I don't see the real value of migrating from ? to optional<> syntax in schema. ? is from the schema side, it is just a short hand of the optional<> syntax is c++ side, they mean the same thing. It was originally designed to use ? to denote the optional types (as the same with bunch of other typed languages), it is just that Tensor? have slightly different meaning because of old TH behaviors. it is also good to keep the schema simple and easier to understand.

Also Tensor? will always accept undefined tensor and this might not going to have any changes, we will not have optional<Tensor> afaik. Keeping the same and one syntax for those kind of stuff will reduce the mental overhead as well.

tugrulates · 2018-12-18T21:37:42Z

Reverted the last commit that changes the schema syntax to optional<T>. I will add the e2e tests with the next PR that migrates the ops to use this (#15133). This one should be ready to go.

facebook-github-bot

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

wanchaol

Nice

Optional ScalarType

a102774

tugrulates requested review from gchanan and zou3519 December 13, 2018 03:33

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Dec 13, 2018

tugrulates changed the title ~~Optional ScalarType~~ Optional ScalarType support for native functions & JIT Dec 13, 2018

tugrulates commented Dec 13, 2018

View reviewed changes

tugrulates requested a review from zdevito December 13, 2018 19:43

gchanan reviewed Dec 13, 2018

View reviewed changes

Remove unused code

d860ea2

facebook-github-bot reviewed Dec 13, 2018

View reviewed changes

zou3519 reviewed Dec 14, 2018

View reviewed changes

wanchaol reviewed Dec 14, 2018

View reviewed changes

zasdfgbnm mentioned this pull request Dec 14, 2018

Add at::one_hot #15208

Closed

facebook-github-bot reviewed Dec 18, 2018

View reviewed changes

wanchaol approved these changes Dec 19, 2018

View reviewed changes

facebook-github-bot closed this in 560530a Dec 19, 2018

tugrulates deleted the optional-scalartype-base branch December 19, 2018 18:58

tugrulates mentioned this pull request Dec 20, 2018

Optional dtype for reduce functions #15133

Closed

zasdfgbnm mentioned this pull request Jan 7, 2019

Improve one_hot #15457

Open

ezyang added the merged label Jun 25, 2019

Conversation

tugrulates commented Dec 13, 2018

Uh oh!

tugrulates Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

tugrulates Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

gchanan Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

tugrulates Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

tugrulates Dec 19, 2018

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zou3519 commented Dec 14, 2018

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

wanchaol left a comment

Choose a reason for hiding this comment

Uh oh!

zasdfgbnm commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tugrulates commented Dec 14, 2018

Uh oh!

tugrulates commented Dec 14, 2018

Uh oh!

tugrulates commented Dec 14, 2018

Uh oh!

wanchaol commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tugrulates commented Dec 18, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

wanchaol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zasdfgbnm commented Dec 14, 2018 •

edited

Loading

wanchaol commented Dec 14, 2018 •

edited

Loading