[pytorch] Add triplet margin loss with custom distance #43680

ethch18 · 2020-08-27T00:52:37Z

Stack from ghstack:

[pytorch] Add triplet margin loss with custom distance #43680 [pytorch] Add triplet margin loss with custom distance

Summary: As discussed here,
adding in a Python-only implementation of the triplet-margin loss that takes a
custom distance function. Still discussing whether this is necessary to add to
PyTorch Core.

Test Plan: python test/run_tests.py

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D23363898

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

dr-ci · 2020-08-27T01:02:18Z

💊 CI failures summary and remediations

As of commit 7756ad2 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

2/2 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

❄️ 2 failures tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_windows_vs2019_py36_cuda10.1_test1 (1/2)

Step: "Test" (full log | diagnosis details | 🔁 rerun) ❄️

ModuleNotFoundError: No module named 'torch'

[ERROR:VsDevCmd.bat] Where [value] is: 
[ERROR:VsDevCmd.bat]    1 : basic debug logging 
[ERROR:VsDevCmd.bat]    2 : detailed debug logging 
[ERROR:VsDevCmd.bat]    3 : trace level logging. Redirection of output to a file when using this level is recommended. 
[ERROR:VsDevCmd.bat] Example: set VSCMD_DEBUG=3 
[ERROR:VsDevCmd.bat]          vsdevcmd.bat > vsdevcmd.trace.txt 2>&1 
Run jit_profiling tests 
Traceback (most recent call last): 
  File "run_test.py", line 13, in <module> 
    import torch 
ModuleNotFoundError: No module named 'torch' 
+ cleanup
+ retcode=1
+ set +x

pytorch_windows_vs2019_py36_cuda10.1_test2 (2/2)

Step: "Test" (full log | diagnosis details | 🔁 rerun) ❄️

ModuleNotFoundError: No module named 'torch'

[ERROR:VsDevCmd.bat] vsdevcmd.bat [args] for additional details. 
[ERROR:VsDevCmd.bat] Where [value] is: 
[ERROR:VsDevCmd.bat]    1 : basic debug logging 
[ERROR:VsDevCmd.bat]    2 : detailed debug logging 
[ERROR:VsDevCmd.bat]    3 : trace level logging. Redirection of output to a file when using this level is recommended. 
[ERROR:VsDevCmd.bat] Example: set VSCMD_DEBUG=3 
[ERROR:VsDevCmd.bat]          vsdevcmd.bat > vsdevcmd.trace.txt 2>&1 
Traceback (most recent call last): 
  File "run_test.py", line 13, in <module> 
    import torch 
ModuleNotFoundError: No module named 'torch' 
+ cleanup
+ retcode=1
+ set +x

1 failure confirmed as flaky and can be ignored:

pytorch_linux_bionic_py3_8_gcc9_build

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 78 times.

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23363898](https://our.internmc.facebook.com/intern/diff/D23363898) [ghstack-poisoned]

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: b62a832 Pull Request resolved: #43680

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23363898](https://our.internmc.facebook.com/intern/diff/D23363898) [ghstack-poisoned]

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 14fbd6d Pull Request resolved: #43680

test/test_nn.py

torch/nn/modules/loss.py

torch/nn/functional.py

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23363898](https://our.internmc.facebook.com/intern/diff/D23363898) [ghstack-poisoned]

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. * Consolidate tests, clarify functional limitations * Documentation updates * Remove stray imports * Fix CI Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: a4e87fa Pull Request resolved: #43680

ethch18 · 2020-09-18T18:41:08Z

Addressed doc comments + rebase + add test decorator. @mruberry @albanD this should be ready to go once CI passes!

ethch18 · 2020-09-18T22:13:14Z

Looks like all CI tests are passing except for a couple (flaky-esque?) windows failures -- is this expected/is there a way to tell if something is broken upstream?

mruberry · 2020-09-18T22:15:34Z

Looks like all CI tests are passing except for a couple (flaky-esque?) windows failures -- is this expected/is there a way to tell if something is broken upstream?

Yep. Sorry for the confusion. Windows CI is broken at the moment and we're working on a fix. You can ignore those failures ;)

ethch18 · 2020-09-18T22:32:24Z

Cool -- in that case, I think this is ready. What steps do I have to complete on my end for the merge?

(if it's preferable, I'm totally fine with waiting until a non-Friday afternoon to merge this too)

mruberry · 2020-09-18T22:33:51Z

Cool -- in that case, I think this is ready. What steps do I have to complete on my end for the merge?

(if it's preferable, I'm totally fine with waiting until a non-Friday afternoon to merge this too)

You don't need to do anything at this point. We take it from here.

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23363898](https://our.internmc.facebook.com/intern/diff/D23363898) [ghstack-poisoned]

Summary: As discussed [here](#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. * Consolidate tests, clarify functional limitations * Documentation updates * Remove stray imports * Fix CI Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 7052170 Pull Request resolved: #43680

mruberry · 2020-09-22T18:54:52Z

This is now merged, congrats @ethch18! This is a cool module.

ethch18 · 2020-09-22T19:16:37Z

Thanks @mruberry and @albanD! Appreciate the mentorship and guidance.

Curious what your thoughts are on the C++ side of this -- I had PR #44072 open as a proof-of-concept but that's pretty out of date at this point. If there's a need, I can continue working on this over the next month or so, or I could hand it off to someone else if that's preferable.

mruberry · 2020-09-22T20:02:04Z

@glaringlee is our C++ API czar. We do like to keep the C++ API consistent with the Python API and would definitely accept a PR implementing this module in C++.

facebook-github-bot · 2020-09-22T20:24:55Z

This pull request has been merged in ef885c1.

facebook-github-bot · 2020-09-22T20:25:00Z

This pull request has been merged in ef885c1.

glaringlee · 2020-09-22T20:33:29Z

@ethch18
I saw PR #44072 is pretty much done. Can you clean it a bit and sync with the python version (if anything changed in pytrhon but not updated in this C++ version), I can help reviewing this and land it. Thank you so much for contributing!

ethch18 · 2020-09-22T20:42:41Z

@glaringlee sure, thanks for reviewing this!. PR #44072 is out of date -- had quite a few changes made here that weren't reflected there. I don't have time at the moment but will tidy it up once I have a chance and ping you in the comments there.

Summary: As discussed [here](pytorch/pytorch#43342), adding in a Python-only implementation of the triplet-margin loss that takes a custom distance function. Still discussing whether this is necessary to add to PyTorch Core. * Consolidate tests, clarify functional limitations * Documentation updates * Remove stray imports * Fix CI Test Plan: python test/run_tests.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 672055f Pull Request resolved: pytorch/pytorch#43680

@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]

@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]

@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]

@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]

@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]

@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]

Summary: Pull Request resolved: #45377 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. Test Plan: Imported from OSS Reviewed By: izdeby Differential Revision: D24003973 fbshipit-source-id: 2d9ada7260a6f27425ff2fdbbf623dad0fb79405

ethch18 requested a review from apaszke as a code owner August 27, 2020 00:52

ethch18 linked an issue Aug 27, 2020 that may be closed by this pull request

Proposal: Generic Triplet-Margin Loss #43342

Closed

ethch18 requested review from albanD and mruberry August 27, 2020 17:15

mruberry removed the request for review from apaszke August 28, 2020 05:39