[c++] Distance-agnostic triplet margin loss #45377

ethch18 · 2020-09-26T18:02:15Z

Stack from ghstack:

[c++] Distance-agnostic triplet margin loss #45377 [c++] Distance-agnostic triplet margin loss

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator.

cc: @mruberry @albanD @glaringlee

Differential Revision: D24003973

[ghstack-poisoned]

dr-ci · 2020-09-26T18:20:58Z

💊 CI failures summary and remediations

As of commit 023aa43 (more details on the Dr. CI page):

1/2 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)
1/2 broken upstream at merge base 7cde662 on Sep 29 from 7:36am to 8:34am PDT (1 commit; 7cde662 - 6d37126)

1 failure confirmed as flaky and can be ignored:

pytorch_xla_linux_bionic_py3_6_clang9_build

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

Since your merge base is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_macos_10_13_py3_build on Sep 29 from 7:36am to 8:34am PDT (1 commit; 7cde662 - 6d37126)
- 🔁 rerun

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.7-py3.6

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 28 times.

@mruberry

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator. cc: @mruberry @albanD @glaringlee [ghstack-poisoned]

ghstack-source-id: 13d5caecceea15fc9c98b8366e15d52c1bd88b83 Pull Request resolved: #45377

glaringlee · 2020-09-28T14:23:47Z

@ethch18
I'll review this soon. Can you do a rebase? It seems the CI tests failed due to a problematic master base.

@mruberry

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator. cc: @mruberry @albanD @glaringlee [ghstack-poisoned]

ghstack-source-id: 2ff77abfce01455332b5905b7bfb246a022cde13 Pull Request resolved: #45377

test/cpp/api/functional.cpp

test/cpp/api/modules.cpp

glaringlee · 2020-09-28T19:27:25Z

torch/csrc/api/src/nn/modules/loss.cpp

@@ -180,6 +180,33 @@ Tensor TripletMarginLossImpl::forward(

 // ============================================================================

+TripletMarginWithDistanceLossImpl::TripletMarginWithDistanceLossImpl(
+    const TripletMarginWithDistanceLossOptions& options_) // NOLINT(modernize-pass-by-value)


Will complier complain if removing NOLINT(...)?
I kinda remember c10:optional doesn't work well with std::move. Did you try std::move here?

clint complains about this otherwise; I copied the NOLINT() from elsewhere in the file.

What would std::move look like in this case? Looks like everything else in this file uses the same format, minus NOLINT() if it's not applicable.

You can do the following to avoid that lint error.
TripletMarginWithDistanceLossImpl::TripletMarginWithDistanceLossImpl( TripletMarginWithDistanceLossOptions options_) : options(std::move(options_)) {}
But I kinda remember std::move is not working well with c10::optional which is inside options. But if you can try, that would be great.

std::move couldn't build once for me locally, then it started building -- not sure if it's a flaky failure. I've pushed this and if the CI fails, then I'll go back to pass-by-reference

@mruberry

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator. cc: @mruberry @albanD @glaringlee [ghstack-poisoned]

ghstack-source-id: 6cd50e4f3c80baa39723fcb129366a442bfa26f1 Pull Request resolved: #45377

codecov · 2020-09-29T09:57:31Z

Codecov Report

Merging #45377 into gh/ethch18/5/base will decrease coverage by 0.40%.
The diff coverage is n/a.

@@                  Coverage Diff                  @@
##           gh/ethch18/5/base   #45377      +/-   ##
=====================================================
- Coverage              68.56%   68.15%   -0.41%     
=====================================================
  Files                    405      396       -9     
  Lines                  51937    51131     -806     
=====================================================
- Hits                   35610    34849     -761     
+ Misses                 16327    16282      -45

Impacted Files	Coverage Δ
torch/optim/sgd.py	`73.91% <0.00%> (-19.57%)`	⬇️
torch/optim/rmsprop.py	`74.57% <0.00%> (-15.26%)`	⬇️
torch/utils/_benchmark/utils/compare.py	`89.50% <0.00%> (-7.84%)`	⬇️
torch/optim/adamw.py	`83.05% <0.00%> (-6.78%)`	⬇️
torch/utils/_benchmark/utils/common.py	`90.90% <0.00%> (-5.50%)`	⬇️
torch/optim/asgd.py	`90.24% <0.00%> (-2.44%)`	⬇️
torch/optim/adadelta.py	`86.04% <0.00%> (-2.33%)`	⬇️
torch/testing/_internal/common_nn.py	`83.33% <0.00%> (-2.21%)`	⬇️
torch/optim/adamax.py	`85.10% <0.00%> (-2.13%)`	⬇️
torch/onnx/symbolic_opset8.py	`28.65% <0.00%> (-1.35%)`	⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cde662...023aa43. Read the comment docs.

glaringlee · 2020-09-29T14:18:52Z

@ethch18
All CI builds passed (rocm test failure is fine). I am going to build this on my end locally, will approve this if it passed. Thanks a lot for contributing.

ethch18 · 2020-09-29T14:55:28Z

@glaringlee great! FYI, Mike asked me to incorporate the changes from PR #45378 into this one too, which I'll do shortly

glaringlee · 2020-09-29T14:57:33Z

@ethch18 sure, go ahead!

@mruberry

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator. cc: @mruberry @albanD @glaringlee [ghstack-poisoned]

ghstack-source-id: 13b4775af1a0e433ec85310c9d127581b0502277 Pull Request resolved: #45377

ethch18 · 2020-09-29T15:46:43Z

@glaringlee updated -- let me know if you need anything else from my end!

glaringlee · 2020-09-29T17:22:40Z

@ethch18
Thanks, can you please rebase your code once? There are several CI test failed, I think that's upstream problems, rebase should solve it.

@mruberry

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator. cc: @mruberry @albanD @glaringlee [ghstack-poisoned]

ghstack-source-id: f0ea5ff8fa766154cb7bf240bc4620ba0b66443f Pull Request resolved: #45377

ethch18 · 2020-09-29T17:33:37Z

@glaringlee Just rebased on fbcode/warm

glaringlee · 2020-09-29T18:59:50Z

@ethch18
Sry, can you rebase again, this code still failed one of our CI test: pytorch_xla_linux_bionic_py3_6_clang9_build.

@mruberry

This PR adds a C++ implementation of the TripletMarginWithDistanceLoss, for which the Python implementation was introduced in PR #43680. It's based on PR #44072, but I'm resubmitting this to unlink it from Phabricator. cc: @mruberry @albanD @glaringlee [ghstack-poisoned]

ethch18 · 2020-09-29T19:09:47Z

@glaringlee rebased with fbcode/warm again. If this doesn't work, I'll try viable/strict next

ghstack-source-id: 9b60cfa8377a8a4dfa60a3e1955eb710881a7f8a Pull Request resolved: #45377

glaringlee · 2020-09-29T19:11:32Z

@ethch18
Thanks, it should work. There is a know issue for our XLA build, it can be solved by rebase.

glaringlee

LGTM now

facebook-github-bot · 2020-10-01T00:52:24Z

This pull request has been merged in c9bb990.

[c++] Distance-agnostic triplet margin loss

08fab1c

[ghstack-poisoned]

ethch18 requested review from ebetica, goldsborough and yf225 as code owners September 26, 2020 18:02

ethch18 mentioned this pull request Sep 26, 2020

[cpp] Add distance-agnostic triplet loss to core #44072

Closed

ethch18 requested a review from glaringlee September 26, 2020 18:06

ethch18 linked an issue Sep 26, 2020 that may be closed by this pull request

Proposal: Generic Triplet-Margin Loss #43342

Closed

ethch18 removed request for ebetica, yf225 and goldsborough September 26, 2020 18:10

pytorchbot added the open source label Sep 26, 2020

ethch18 added a commit that referenced this pull request Sep 26, 2020

[c++] Distance-agnostic triplet margin loss

6a8dfc9

ghstack-source-id: 13d5caecceea15fc9c98b8366e15d52c1bd88b83 Pull Request resolved: #45377

ethch18 mentioned this pull request Sep 27, 2020

[docs] Make spelling of 'nonnegative' consistent in TripletMarginWithDistanceLoss #45378

Closed

gchanan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 28, 2020

ethch18 added a commit that referenced this pull request Sep 28, 2020

[c++] Distance-agnostic triplet margin loss

07abdbd

ghstack-source-id: 2ff77abfce01455332b5905b7bfb246a022cde13 Pull Request resolved: #45377

glaringlee reviewed Sep 28, 2020

View reviewed changes

ethch18 added a commit that referenced this pull request Sep 29, 2020

[c++] Distance-agnostic triplet margin loss

f41e2a6

ghstack-source-id: 6cd50e4f3c80baa39723fcb129366a442bfa26f1 Pull Request resolved: #45377

ethch18 requested a review from apaszke as a code owner September 29, 2020 15:42

ethch18 added a commit that referenced this pull request Sep 29, 2020

[c++] Distance-agnostic triplet margin loss

8a134ca

ghstack-source-id: 13b4775af1a0e433ec85310c9d127581b0502277 Pull Request resolved: #45377

ethch18 removed the request for review from apaszke September 29, 2020 15:43

ethch18 added a commit that referenced this pull request Sep 29, 2020

[c++] Distance-agnostic triplet margin loss

3716530

ghstack-source-id: f0ea5ff8fa766154cb7bf240bc4620ba0b66443f Pull Request resolved: #45377

ethch18 added a commit that referenced this pull request Sep 29, 2020

[c++] Distance-agnostic triplet margin loss

719406a

ghstack-source-id: 9b60cfa8377a8a4dfa60a3e1955eb710881a7f8a Pull Request resolved: #45377

glaringlee approved these changes Sep 30, 2020

View reviewed changes

facebook-github-bot closed this in c9bb990 Sep 30, 2020

facebook-github-bot added the merged label Oct 1, 2020

facebook-github-bot deleted the gh/ethch18/5/head branch October 4, 2020 14:18

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++] Distance-agnostic triplet margin loss #45377

[c++] Distance-agnostic triplet margin loss #45377

ethch18 commented Sep 26, 2020 •

edited by glaringlee

dr-ci bot commented Sep 26, 2020 •

edited

glaringlee commented Sep 28, 2020 •

edited

glaringlee Sep 28, 2020

ethch18 Sep 28, 2020

glaringlee Sep 28, 2020

ethch18 Sep 29, 2020

codecov bot commented Sep 29, 2020 •

edited

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020 •

edited

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020

glaringlee commented Sep 29, 2020

glaringlee left a comment

facebook-github-bot commented Oct 1, 2020

[c++] Distance-agnostic triplet margin loss #45377

[c++] Distance-agnostic triplet margin loss #45377

Conversation

ethch18 commented Sep 26, 2020 • edited by glaringlee

dr-ci bot commented Sep 26, 2020 • edited

💊 CI failures summary and remediations

🚧 1 fixed upstream failure:

ci.pytorch.org: 1 failed

glaringlee commented Sep 28, 2020 • edited

glaringlee Sep 28, 2020

Choose a reason for hiding this comment

ethch18 Sep 28, 2020

Choose a reason for hiding this comment

glaringlee Sep 28, 2020

Choose a reason for hiding this comment

ethch18 Sep 29, 2020

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2020 • edited

Codecov Report

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020 • edited

glaringlee commented Sep 29, 2020

ethch18 commented Sep 29, 2020

glaringlee commented Sep 29, 2020

glaringlee left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2020

ethch18 commented Sep 26, 2020 •

edited by glaringlee

dr-ci bot commented Sep 26, 2020 •

edited

glaringlee commented Sep 28, 2020 •

edited

codecov bot commented Sep 29, 2020 •

edited

ethch18 commented Sep 29, 2020 •

edited