Use c10::variant for enums in C++ API by yf225 · Pull Request #26837 · pytorch/pytorch

yf225 · 2019-09-25T20:19:24Z

Stack from ghstack:

Use c10::variant-based enums for Nonlinearity and FanMode #27933 Use c10::variant-based enums for Nonlinearity and FanMode
Use c10::variant for enums in C++ API #26837 Use c10::variant for enums in C++ API
Add std::variant backport as c10::variant #26836 Add std::variant backport as c10::variant

The discussion in #15149 captures it the best, and there are two main benefits of using c10::variant:

Compared to defining a common enum class for all functions, using c10::variant we can constrain the the range of acceptable enums for a particular function.
Compared to using std::string as enum, using c10::variant we can save the string comparison overhead, and also constrain the the range of acceptable enums for a particular function.

Differential Revision: D17579438

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

yf225 · 2019-09-25T20:31:29Z

@ShahriarSS Would you like to review this PR?

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ShahriarRezghi · 2019-09-25T21:04:35Z

@yf225 I think you forgot about this one and this one (Although the second one is in detail namespace so maybe it's not that important).

yf225 · 2019-09-25T21:09:53Z

@ShahriarSS Yes those are skipped intentionally because we likely need to rewrite how RNN are implemented in C++ API. I will fix the enum issue when I work on parity for the RNN layers.

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ShahriarRezghi · 2019-09-25T21:18:34Z

@yf225 Then this looks good to me. Just one thing: Is it a bad idea to have a macro that defines the enum and it's extern const like TORCH_ARG or TORCH_MODULE (is it even possible)?

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

yf225 · 2019-09-25T21:47:22Z

@ShahriarSS Using macro is a great idea - I will add it in this PR.

smessmer · 2019-09-25T21:47:52Z

Can you give some background on why we want to do this? enum class seems like less effort to use.

yf225 · 2019-09-25T21:56:25Z

@smessmer I think the discussion in #15149 captures it the best, and I think there are three main benefits of using c10::variant:

Compared to defining an enum class for each function, using c10::variant we can declare the range of acceptable enums inline with the function declaration, which makes the code easier to read.
Compared to defining a common enum class for all functions, using c10::variant we can constrain the the range of acceptable enums for a particular function.
Compared to using std::string as enum, using c10::variant we can save the string comparison overhead, and also constrain the the range of acceptable enums for a particular function.

I will update the PR description to reflect this rationale.

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ShahriarRezghi · 2019-09-26T14:18:31Z

torch/csrc/api/include/torch/enum.h

+namespace torch {
+
+/// Variable of `Nonlinearity` type can take one of the following values:
+/// - `torch::kLinear`


@yf225 Aren't these comments redundant? the accepted values are defined in the body of declaration itself.

I am a bit worried that people won't directly see the mapping from enumtype::Linear to torch::kLinear, and this comment should help them discover that.

@ShahriarSS I changed the enum type from enumtype::Linear to enumtype::torch::kLinear, so that people can more easily figure out that they should use torch::kLinear.

ShahriarRezghi · 2019-09-26T14:18:52Z

torch/csrc/api/include/torch/enum.h

+>;
+
+/// Variable of `FanMode` type can take one of the following values:
+/// - `torch::kFanIn`


Same thing here about being redundant.

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ezyang · 2019-10-15T13:50:06Z

c10/util/variant.h


 #ifdef MPARK_VARIABLE_TEMPLATES
-  constexpr variant_in_place_t in_place{};
+  constexpr variant_in_place_t variant_in_place{};


What's going on here?

It's for avoiding conflict with the same variable name in c10/util/Optional.h. I added a comment in the c10-specific change log for better clarity.

We should just make variant.h use the optional.h definition. in_place is part of the standard.

I moved the common definition for in_place into c10/util/in_place.h, so that both variant.h and optional.h can use it.

ezyang · 2019-10-15T13:52:07Z

torch/csrc/api/include/torch/enum.h

+
+#define TORCH_ENUM_DECLARE(name) \
+namespace torch { \
+namespace enumtype { \


How'd you decide on this name? Is it just an internal name or is it visible to users?

It is only an internal name - the type of the enum is e.g. torch::enumtype::kFanIn, and the variable that has this type is torch::kFanIn, which is what's used by the end user.

torch/csrc/api/include/torch/enum.h

ezyang · 2019-10-15T13:56:38Z

The way of referencing the enums got a lot wordier in this PR. Was this intentional?

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

yf225 · 2019-10-15T16:30:02Z

The way of referencing the enums got a lot wordier in this PR. Was this intentional?

Yes it was intentional - they will be changed to the short form in #27933.

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

yf225 · 2019-10-16T21:03:53Z

@pytorchbot rebase this please

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

facebook-github-bot · 2019-10-17T07:12:02Z

@yf225 merged this pull request in aad5071.

Summary: This PR adds temporary declarations for `torch::k{name}` enums, so that we can submit a PR to rename the enum usage in torchvision. And then, after the changes to torchvision is done, we can remove the temporary declarations in pytorch#26837 to officially move over to using `c10::variant` for enums. Pull Request resolved: pytorch#27051 Differential Revision: D17672220 Pulled By: yf225 fbshipit-source-id: 4ae77634e8c7efa3404698f7c1a69177cbb5dab3

Summary: Pull Request resolved: pytorch#26837 Test Plan: Imported from OSS Differential Revision: D17579438 Pulled By: yf225 fbshipit-source-id: 9ac59df28a317fdb3be2cc02c65962ad99117127

[BC-breaking] Use torch::variant for enums in C++ API

5ce469e

yf225 requested review from ebetica and goldsborough as code owners September 25, 2019 20:19

pytorchbot added the module: cpp Related to C++ API label Sep 25, 2019

yf225 mentioned this pull request Sep 25, 2019

Add std::variant backport as c10::variant #26836

Closed

Update on "[BC-breaking] Use torch::variant for enums in C++ API"

fb1dabd

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Will Feng added 5 commits September 25, 2019 16:40

Update on "[BC-breaking] Use torch::variant for enums in C++ API"

38a9779

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use torch::variant for enums in C++ API"

1fb4dcb

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use torch::variant for enums in C++ API"

b70e28c

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use torch::variant for enums in C++ API"

ab867bb

[BC-breaking] Use torch::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use torch::variant for enums in C++ API"

23c17a2

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

yf225 changed the title ~~[BC-breaking] Use torch::variant for enums in C++ API~~ [BC-breaking] Use c10::variant for enums in C++ API Sep 25, 2019

Will Feng added 2 commits September 25, 2019 16:52

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

7241756

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

740a63f

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

9064981

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

0bd90cb

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

7dc68a3

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

pytorchbot added caffe2 module: build Build system issues labels Sep 25, 2019

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

12967ec

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ShahriarRezghi reviewed Sep 26, 2019

View reviewed changes

yf225 mentioned this pull request Sep 26, 2019

experiment #26895

Closed

Update on "[BC-breaking] Use c10::variant for enums in C++ API"

ed18e65

[BC-breaking] Use c10::variant for enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Will Feng added 4 commits October 14, 2019 20:57

Update on "Use c10::variant for enums in C++ API"

da1197d

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

c622507

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

d570b19

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

e1f7a06

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ezyang reviewed Oct 15, 2019

View reviewed changes

torch/csrc/api/include/torch/enum.h Outdated Show resolved Hide resolved

Update on "Use c10::variant for enums in C++ API"

9aa66bc

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

yf225 requested a review from ezyang October 15, 2019 16:30

Update on "Use c10::variant for enums in C++ API"

e212e87

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

ezyang approved these changes Oct 16, 2019

View reviewed changes

Will Feng added 5 commits October 16, 2019 14:03

Update on "Use c10::variant for enums in C++ API"

38b1382

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

ff77083

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

4e4df1b

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

7d45d5b

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

c133245

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Will Feng added 2 commits October 16, 2019 18:01

Update on "Use c10::variant for enums in C++ API"

e51d1a2

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

Update on "Use c10::variant for enums in C++ API"

b7a89f3

Add c10::variant-based enums in C++ API gh-metadata: pytorch pytorch 26837 gh/yf225/7/head

facebook-github-bot closed this in aad5071 Oct 17, 2019

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

facebook-github-bot deleted the gh/yf225/7/head branch October 28, 2019 22:22

mruberry added the Merged label Oct 28, 2020

Conversation

yf225 commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented Sep 25, 2019

Uh oh!

ShahriarRezghi commented Sep 25, 2019

Uh oh!

yf225 commented Sep 25, 2019

Uh oh!

ShahriarRezghi commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented Sep 25, 2019

Uh oh!

smessmer commented Sep 25, 2019

Uh oh!

yf225 commented Sep 25, 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!

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!

Uh oh!

ezyang commented Oct 15, 2019

Uh oh!

yf225 commented Oct 15, 2019

Uh oh!

yf225 commented Oct 16, 2019

Uh oh!

facebook-github-bot commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yf225 commented Sep 25, 2019 •

edited

Loading

ShahriarRezghi commented Sep 25, 2019 •

edited

Loading

yf225 commented Sep 25, 2019 •

edited

Loading