[jit] Implement `torch::jit::deregisterOperator` #35107

malfet · 2020-03-20T18:29:35Z

Test Plan: CI + bin/test_jit --gtest_filter=JitTest.CustomOperators --gtest_repeat=2

dr-ci · 2020-03-20T18:54:57Z

💊 CircleCI build failures summary and remediations

As of commit 09d452a (more details on the Dr. CI page):

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following build failures do not appear to be due to upstream breakages (reran 1 job to discount flakiness):

pytorch_linux_xenial_py3_6_gcc5_4_test (1/1)

Step: "Test" (full log | pattern match details) <confirmed not flaky by 2 failures>

Mar 26 01:32:58 AssertionError: 1 not less than or equal to 1e-05 :

Mar 26 01:32:58   File "test_quantization.py", line 1358, in test_per_tensor_dynamic_quant_observers 
Mar 26 01:32:58     min_side=1, max_side=10), 
Mar 26 01:32:58   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/hypothesis/core.py", line 1116, in wrapped_test 
Mar 26 01:32:58     raise the_error_hypothesis_found 
Mar 26 01:32:58   File "test_quantization.py", line 1373, in test_per_tensor_dynamic_quant_observers 
Mar 26 01:32:58     self.assertEqual(ref[1], qparams[1]) 
Mar 26 01:32:58   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 795, in assertEqual 
Mar 26 01:32:58     allow_inf=allow_inf, exact_dtype=exact_dtype) 
Mar 26 01:32:58   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 915, in assertEqual 
Mar 26 01:32:58     super(TestCase, self).assertLessEqual(abs(x - y), prec, message) 
Mar 26 01:32:58 AssertionError: 1 not less than or equal to 1e-05 :  
Mar 26 01:32:58  
Mar 26 01:32:58 ---------------------------------------------------------------------- 
Mar 26 01:32:58 Ran 39 tests in 56.339s 
Mar 26 01:32:58  
Mar 26 01:32:58 FAILED (failures=1, skipped=2) 
Mar 26 01:32:58  
Mar 26 01:32:58 Generating XML reports... 
Mar 26 01:32:58 Generated XML report: test-reports/python-unittest/TEST-EagerModePostTrainingQuantTest-20200326013202.xml 
Mar 26 01:32:58 Generated XML report: test-reports/python-unittest/TEST-EagerModeQuantizationAwareTrainingTest-20200326013202.xml 
Mar 26 01:32:58 Generated XML report: test-reports/python-unittest/TEST-FunctionalModuleTest-20200326013202.xml

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.

See how this bot performed.

This comment has been revised 15 times.

malfet · 2020-03-20T21:06:57Z

Modified diff to workaround global destructor order ambiguity issue.

ezyang · 2020-03-23T21:30:12Z

This looks reasonable but you should get a review from someone who has actually touched this code before.

bhosmer · 2020-03-23T21:56:06Z

cc @smessmer, how would this interact with c10/jit registry syncing via listener etc.?

smessmer

Thank you very much for implementing this, this has been long overdue.

cc @smessmer, how would this interact with c10/jit registry syncing via listener etc.?

That's the line I commented on. This should finally make c10 deregistrations work in a sound way (i.e. call through to also deregister the wrapper from JIT).

torch/csrc/jit/runtime/register_c10_ops.cpp

torch/csrc/jit/runtime/operator.cpp

facebook-github-bot

@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

bhosmer · 2020-03-25T20:26:01Z

That's the line I commented on. This should finally make c10 deregistrations work in a sound way (i.e. call through to also deregister the wrapper from JIT).

Yeah that's very cool :) also wanted to double check that c10 isn't depending on a critical way on the invariant that ops registered to the jit via listener are definitely present, given that this can be called unilaterally, but it sounds like we're not :)

facebook-github-bot

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

Test Plan: CI + `bin/test_jit --gtest_filter=JitTest.CustomOperators --gtest_repeat=2`

facebook-github-bot

@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

smessmer · 2020-03-26T18:04:07Z

aten/src/ATen/core/dispatch/Dispatcher.cpp

+    auto raw_ptr = listener.get();
    listeners_.push_back(std::move(listener));
+    return RegistrationHandleRAII([this, raw_ptr] {
+        const auto& found_it = std::find_if(listeners_.begin(), listeners_.end(), [raw_ptr](auto& it) {


Instead of calling find() here, which could be slow, you could make listeners_ a std::list and just remember the iterator in the closure. std::list keeps iterators valid, so that's not an issue.

Will do in follow up PR

malfet requested review from smessmer and suo March 20, 2020 18:29

malfet requested a review from apaszke as a code owner March 20, 2020 18:29

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 20, 2020

malfet linked an issue Mar 20, 2020 that may be closed by this pull request

bin/test_jit --gtest_filter=JitTest.CustomOperators --gtest_repeat=2 fails #35103

Closed

malfet force-pushed the malfet/add-torch-jit-deregister-operator branch from 9187879 to 3ef65d2 Compare March 20, 2020 21:03

malfet requested a review from ezyang March 20, 2020 21:05

malfet force-pushed the malfet/add-torch-jit-deregister-operator branch from 3ef65d2 to 821f3fd Compare March 25, 2020 03:10

smessmer approved these changes Mar 25, 2020

View reviewed changes

torch/csrc/jit/runtime/register_c10_ops.cpp Outdated Show resolved Hide resolved

smessmer reviewed Mar 25, 2020

View reviewed changes

torch/csrc/jit/runtime/operator.cpp Outdated Show resolved Hide resolved

facebook-github-bot reviewed Mar 25, 2020

View reviewed changes

malfet force-pushed the malfet/add-torch-jit-deregister-operator branch from 821f3fd to 4b5fce4 Compare March 25, 2020 23:45

facebook-github-bot reviewed Mar 25, 2020

View reviewed changes

[jit] Implement torch::jit::deregisterOperator

09d452a

Test Plan: CI + `bin/test_jit --gtest_filter=JitTest.CustomOperators --gtest_repeat=2`

malfet force-pushed the malfet/add-torch-jit-deregister-operator branch from 4b5fce4 to 09d452a Compare March 25, 2020 23:59

facebook-github-bot reviewed Mar 26, 2020

View reviewed changes

facebook-github-bot closed this in be0cdf5 Mar 26, 2020

malfet deleted the malfet/add-torch-jit-deregister-operator branch March 26, 2020 17:57

smessmer reviewed Mar 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[jit] Implement `torch::jit::deregisterOperator` #35107

[jit] Implement `torch::jit::deregisterOperator` #35107

malfet commented Mar 20, 2020

Uh oh!

dr-ci bot commented Mar 20, 2020 •

edited

Loading

Uh oh!

malfet commented Mar 20, 2020

Uh oh!

ezyang commented Mar 23, 2020

Uh oh!

bhosmer commented Mar 23, 2020

Uh oh!

smessmer left a comment

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Uh oh!

bhosmer commented Mar 25, 2020

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot left a comment

Uh oh!

smessmer Mar 26, 2020

Uh oh!

malfet Mar 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[jit] Implement torch::jit::deregisterOperator #35107

[jit] Implement torch::jit::deregisterOperator #35107

Conversation

malfet commented Mar 20, 2020

Uh oh!

dr-ci bot commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_linux_xenial_py3_6_gcc5_4_test (1/1)

Uh oh!

malfet commented Mar 20, 2020

Uh oh!

ezyang commented Mar 23, 2020

Uh oh!

bhosmer commented Mar 23, 2020

Uh oh!

smessmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

bhosmer commented Mar 25, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

smessmer Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

malfet Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[jit] Implement `torch::jit::deregisterOperator` #35107

[jit] Implement `torch::jit::deregisterOperator` #35107

dr-ci bot commented Mar 20, 2020 •

edited

Loading