Wrapping namespace Reduction in namespace at (#26606) #27422

divyanshsinghvi · 2019-10-05T12:47:27Z

Wrapped namespace Reduction in namespace at
Prefixed at:: wherever Reduction:: is used

aten/src/ATen/core/Reduction.h

yf225

Thanks so much for the fix @divyanshsinghvi! I left some very minor comments regarding using torch:: in C++ frontend.

yf225 · 2019-10-07T18:13:23Z

test/test_cpp_api_parity.py

For C++ frontend we encourage using torch:: namespace instead of at::, and it would be great to change all of the use sites here to torch::.

yf225 · 2019-10-07T18:14:19Z

torch/csrc/api/include/torch/nn/options/loss.h

Ditto in this file regarding changing at:: to torch::.

I may be wrong in my understanding. But I was amazed at how the code would work with torch:: instead of at::. So, I tried to dig deeper and found the types.h class which wraps the object with torch namespace and with statement using namespace at we are removing the need for the use of at:: prefix. Please do correct me if my understanding is wrong.

Your understanding is 100% correct. :D For the C++ frontend we only support using torch:: namespace, as using at:: could get users into a lot of Variable vs. Tensor troubles.

yf225 · 2019-10-07T18:22:11Z

@ailzhang XLA build seems to fail with

Oct 05 20:02:56 torch_xla/csrc/tensor_ops.cpp:74:23: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:56   return reduction == Reduction::Mean
Oct 05 20:02:56                       ^~~~~~~~~
Oct 05 20:02:56                       at::Reduction
Oct 05 20:02:56 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:56 namespace Reduction {
Oct 05 20:02:56           ^
Oct 05 20:02:56 torch_xla/csrc/tensor_ops.cpp:107:10: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:56     case Reduction::None:
Oct 05 20:02:56          ^~~~~~~~~
Oct 05 20:02:56          at::Reduction
Oct 05 20:02:56 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:56 namespace Reduction {
Oct 05 20:02:56           ^
Oct 05 20:02:56 torch_xla/csrc/tensor_ops.cpp:109:10: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:56     case Reduction::Mean:
Oct 05 20:02:56          ^~~~~~~~~
Oct 05 20:02:56          at::Reduction
Oct 05 20:02:56 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:56 namespace Reduction {
Oct 05 20:02:56           ^
Oct 05 20:02:56 torch_xla/csrc/tensor_ops.cpp:112:10: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:56     case Reduction::Sum:
Oct 05 20:02:56          ^~~~~~~~~
Oct 05 20:02:56          at::Reduction
Oct 05 20:02:56 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:56 namespace Reduction {
Oct 05 20:02:56           ^
Oct 05 20:02:56 torch_xla/csrc/tensor_ops.cpp:142:10: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:56     case Reduction::None:
Oct 05 20:02:56          ^~~~~~~~~
Oct 05 20:02:56          at::Reduction
Oct 05 20:02:56 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:56 namespace Reduction {
Oct 05 20:02:56           ^
Oct 05 20:02:56 torch_xla/csrc/tensor_ops.cpp:143:10: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:56     case Reduction::Sum:
Oct 05 20:02:56          ^~~~~~~~~
Oct 05 20:02:56          at::Reduction
Oct 05 20:02:57 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:57 namespace Reduction {
Oct 05 20:02:57           ^
Oct 05 20:02:57 torch_xla/csrc/tensor_ops.cpp:145:10: error: use of undeclared identifier 'Reduction'; did you mean 'at::Reduction'?
Oct 05 20:02:57     case Reduction::Mean: {
Oct 05 20:02:57          ^~~~~~~~~
Oct 05 20:02:57          at::Reduction
Oct 05 20:02:57 /opt/conda/lib/python3.6/site-packages/torch/include/ATen/core/Reduction.h:4:11: note: 'at::Reduction' declared here
Oct 05 20:02:57 namespace Reduction {
Oct 05 20:02:57           ^
Oct 05 20:03:02 7 errors generated.

because we are moving Reduction into at:: namespace. Do you think we should issue a quick fix to XLA after merging this PR, or should we have a bridging PR that alias at::Reduction to Reduction first, and remove the alias after XLA is migrated?

yf225 · 2019-10-07T23:26:05Z

NOTE: if #27345 is landed first, we will need to change Reduction usage in torch::nn::CosineEmbeddingLoss as well.

yf225 · 2019-10-08T18:32:23Z

@pytorchbot rebase this please

yf225 · 2019-10-08T18:33:44Z

@divyanshsinghvi #27345 is recently landed which has another occurrence of Reduction (in torch/csrc/api/include/torch/nn/options/loss.h), and we might need to change it as well. Thanks so much for your help!

divyanshsinghvi · 2019-10-08T20:07:32Z

@divyanshsinghvi #27345 is recently landed which has another occurrence of Reduction (in torch/csrc/api/include/torch/nn/options/loss.h), and we might need to change it as well. Thanks so much for your help!

Yes, this PR includes changes for this.

yf225 · 2019-10-08T20:27:23Z

@divyanshsinghvi Awesome thanks a lot! After CI passes I will work with @ailzhang to land this PR and also fix XLA build at the same time. :)

divyanshsinghvi · 2019-10-09T20:00:34Z

@yf225 I think the two fails are now because of xla. According to me, these three files would require the changes in xla repository.
test/cpp/test_aten_xla_tensor.cpp
torch_xla/csrc/aten_xla_type.cpp
torch_xla/csrc/tensor_ops.cpp

yf225 · 2019-10-09T20:10:57Z

@divyanshsinghvi Yes I think so - I just opened a PR to the XLA repo: pytorch/xla#1176, and we will merge that immediately after merging this PR to unbreak XLA build.

facebook-github-bot

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

yf225

@divyanshsinghvi Thanks so much for the awesome work!

yf225 · 2019-10-14T22:18:56Z

I will merge this PR after internal CI passes.

facebook-github-bot

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

divyanshsinghvi · 2019-10-14T23:54:19Z

test/cpp/api/modules.cpp

@yf225 why is it at:: instead of torch::

@divyanshsinghvi thanks a lot for the catch! I changed it to torch::

1)Wrapped namespace Reduction in namespace at 2)Prefixed at:: wherever Reduction:: is used except C++ frontent 3)Prefixed torch:: in C++ frontend

This reverts commit 60435b0.

yf225 · 2019-10-15T01:09:24Z

@divyanshsinghvi It seems that some unrelated commits were accidentally added into this PR branch, so I cleaned them up and rebase the PR on top of current master.

facebook-github-bot

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

divyanshsinghvi · 2019-10-15T07:02:02Z

@divyanshsinghvi It seems that some unrelated commits were accidentally added into this PR branch, so I cleaned them up and rebase the PR on top of current master.

Yes, thanks for cleaning them up.

facebook-github-bot

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

Summary: 1) Wrapped namespace `Reduction` in namespace `at` 2) Prefixed `at::` wherever `Reduction::` is used Pull Request resolved: pytorch/pytorch#27422 Differential Revision: D17913759 Pulled By: yf225 fbshipit-source-id: 8f00ca01cad2e7f673d316b128abf59c026e216c

facebook-github-bot · 2019-10-15T23:47:35Z

@yf225 merged this pull request in 3397d41.

…#27422) Summary: 1) Wrapped namespace `Reduction` in namespace `at` 2) Prefixed `at::` wherever `Reduction::` is used Pull Request resolved: pytorch#27422 Differential Revision: D17913759 Pulled By: yf225 fbshipit-source-id: 8f00ca01cad2e7f673d316b128abf59c026e216c

divyanshsinghvi changed the title ~~Wrapping namespace Reduction in namespace at(#26606)~~ Wrapping namespace Reduction in namespace at (#26606) Oct 5, 2019

divyanshsinghvi marked this pull request as ready for review October 5, 2019 14:44

divyanshsinghvi requested review from apaszke, ebetica, goldsborough and yf225 as code owners October 5, 2019 14:44

divyanshsinghvi mentioned this pull request Oct 5, 2019

namespace Reduction is not wrapped in torch namespace (C++ API) #26606

Closed

divyanshsinghvi force-pushed the Reduction branch from da351de to 6fb7903 Compare October 5, 2019 19:41

yf225 requested a review from li-roy October 7, 2019 04:51

yf225 reviewed Oct 7, 2019

View reviewed changes

aten/src/ATen/core/Reduction.h Outdated Show resolved Hide resolved

li-roy approved these changes Oct 7, 2019

View reviewed changes

yf225 requested changes Oct 7, 2019

View reviewed changes

divyanshsinghvi force-pushed the Reduction branch from 6fb7903 to c23d8fd Compare October 8, 2019 18:27

yf225 mentioned this pull request Oct 9, 2019

Wrapping namespace Reduction in namespace at pytorch/xla#1176

Merged

divyanshsinghvi requested a review from yf225 October 11, 2019 22:30

facebook-github-bot reviewed Oct 14, 2019

View reviewed changes

yf225 approved these changes Oct 14, 2019

View reviewed changes

facebook-github-bot reviewed Oct 14, 2019

View reviewed changes

divyanshsinghvi commented Oct 14, 2019

View reviewed changes

divyanshsinghvi and others added 6 commits October 14, 2019 21:03

Wrapping namespace Reduction in namespace at(pytorch#26606)

150f99e

1)Wrapped namespace Reduction in namespace at 2)Prefixed at:: wherever Reduction:: is used except C++ frontent 3)Prefixed torch:: in C++ frontend

fix another Reduction use site

fbb8547

Wrapping namespace Reduction in namespace at(pytorch#26606)

60435b0

1)Wrapped namespace Reduction in namespace at 2)Prefixed at:: wherever Reduction:: is used except C++ frontent 3)Prefixed torch:: in C++ frontend

Fix New Reduction Use Cases

b932b67

Revert "Wrapping namespace Reduction in namespace at(pytorch#26606)"

8ff5a80

This reverts commit 60435b0.

use torch instead of at

a4b22fb

yf225 force-pushed the Reduction branch from 4e6da23 to a4b22fb Compare October 15, 2019 01:05

facebook-github-bot reviewed Oct 15, 2019

View reviewed changes

Merge branch 'master' into Reduction

0f308d1

facebook-github-bot reviewed Oct 15, 2019

View reviewed changes

This was referenced Oct 15, 2019

[C++ API parity] Soft Margin loss #27660

Closed

[C++ API parity] Multi Label Margin loss #27659

Closed

facebook-github-bot closed this in 3397d41 Oct 15, 2019

divyanshsinghvi deleted the Reduction branch October 15, 2019 21:08

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

mruberry added the Merged label Oct 28, 2020

dhern023 mentioned this pull request Jun 3, 2023

Get Chapter 11 to build using the latest libtorch PacktPublishing/Hands-On-Machine-Learning-with-CPP#11

Open

Wrapping namespace Reduction in namespace at (#26606) #27422

Wrapping namespace Reduction in namespace at (#26606) #27422

Uh oh!

Conversation

divyanshsinghvi commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

divyanshsinghvi Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 7, 2019

Uh oh!

yf225 commented Oct 7, 2019

Uh oh!

yf225 commented Oct 8, 2019

Uh oh!

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

Uh oh!

Uh oh!

divyanshsinghvi commented Oct 8, 2019

Uh oh!

yf225 commented Oct 8, 2019

Uh oh!

divyanshsinghvi commented Oct 9, 2019

Uh oh!

yf225 commented Oct 9, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 14, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

divyanshsinghvi Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 15, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

divyanshsinghvi commented Oct 15, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

divyanshsinghvi commented Oct 5, 2019 •

edited

Loading

divyanshsinghvi Oct 8, 2019 •

edited

Loading

yf225 Oct 8, 2019 •

edited

Loading

yf225 commented Oct 8, 2019 •

edited

Loading

divyanshsinghvi Oct 14, 2019 •

edited

Loading