Skip to content

Add c10::optional to type syntax#12582

Closed
wanchaol wants to merge 11 commits intopytorch:masterfrom
wanchaol:atoptional
Closed

Add c10::optional to type syntax#12582
wanchaol wants to merge 11 commits intopytorch:masterfrom
wanchaol:atoptional

Conversation

@wanchaol
Copy link
Collaborator

@wanchaol wanchaol commented Oct 11, 2018

This PR adds optional type to ATen native, autograd, JIT schema and Python Arg parser, closes #9513. It allows us to use optional default values (including None) for function signature and implementations like clamp, etc., and also let us remove the python_default_init hack.

Follow up:

remove python_default_init completely.

@wanchaol wanchaol force-pushed the atoptional branch 2 times, most recently from 2672bad to 6b10158 Compare October 13, 2018 00:38
@wanchaol wanchaol added the oncall: jit Add this issue/PR to JIT oncall triage queue label Oct 15, 2018
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

Looks good! I have only minor nits.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@wanchaol wanchaol force-pushed the atoptional branch 2 times, most recently from 8cc7232 to bb9e4b9 Compare October 16, 2018 20:47
@wanchaol wanchaol changed the title [WIP] Add at::optional to type syntax Add at::optional to type syntax Oct 16, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@wanchaol
Copy link
Collaborator Author

@pytorchbot retest this please

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@goldsborough goldsborough left a comment

Choose a reason for hiding this comment

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

Looks good

@wanchaol wanchaol changed the title Add at::optional to type syntax Add c10::optional to type syntax Oct 23, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@wanchaol wanchaol force-pushed the atoptional branch 3 times, most recently from 5887281 to ba14796 Compare October 24, 2018 06:08
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@wanchaol
Copy link
Collaborator Author

@pytorchbot retest this please

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@wanchaol wanchaol deleted the atoptional branch October 25, 2018 23:25
zdevito pushed a commit to zdevito/ATen that referenced this pull request Oct 26, 2018
Summary:
This PR adds optional type to ATen native, autograd, JIT schema and Python Arg parser, closes #9513. It allows us to use optional default values (including None) for function signature and implementations like clamp, etc., and also let us remove the python_default_init hack.

Follow up:

remove python_default_init completely.
Pull Request resolved: pytorch/pytorch#12582

Differential Revision: D10417423

Pulled By: wanchaol

fbshipit-source-id: 1c80f0727bb528188b47c595629e2996be269b89
facebook-github-bot pushed a commit that referenced this pull request Dec 19, 2018
Summary:
For #6593 and #9515

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

Note: Mostly following the existing implementation for optional<Scalar> 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.
Pull Request resolved: #15154

Differential Revision: D13457760

Pulled By: tugrulates

fbshipit-source-id: 706134f0bd578683edd416b96329b49a1ba8ab48
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jit] support at::optional

5 participants