Reduce code duplication in OperatorEntry by keying hash map on optional<DispatchKey> #33817

ezyang · 2020-02-26T17:03:17Z

Stack from ghstack:

Delete OperatorOptions, absorb AliasAnalysisKind into FunctionSchema. #34160 Delete OperatorOptions, absorb AliasAnalysisKind into FunctionSchema.
Beef up documentation on Dispatcher.h, reorder methods for clarity. #33838 Beef up documentation on Dispatcher.h, reorder methods for clarity.
Reduce code duplication in OperatorEntry by keying hash map on optional<DispatchKey> #33817 Reduce code duplication in OperatorEntry by keying hash map on optional

Then, nullopt denotes catch all, whereas everything else is specific to
a DispatchKey. I can delete the second copy of methods when I do this.
This refactor should be pushed all the way to the frontend but I am doing
it one step at a time.

Signed-off-by: Edward Z. Yang ezyang@fb.com

Differential Revision: D20125163

…al<DispatchKey> Then, nullopt denotes catch all, whereas everything else is specific to a DispatchKey. I can delete the second copy of methods when I do this. This refactor should be pushed all the way to the frontend but I am doing it one step at a time. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

…al<DispatchKey> Then, nullopt denotes catch all, whereas everything else is specific to a DispatchKey. I can delete the second copy of methods when I do this. This refactor should be pushed all the way to the frontend but I am doing it one step at a time. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 85a6be7 Pull Request resolved: #33817

dr-ci · 2020-02-26T17:15:59Z

💊 CircleCI build failures summary and remediations

As of commit 70ec5ef (more details on the Dr. CI page):

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

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

❄️ 2 tentatively flaky failures

2 failures tentatively classified as flaky but have not launched reruns to confirm:

pytorch_linux_xenial_cuda10_1_cudnn7_py3_multigpu_test (1/2)

Step: "Test" (full log | pattern match details) ❄️

Mar 03 23:12:38 RuntimeError: Error downloading resource!

Mar 03 23:12:38  
Mar 03 23:12:38 During handling of the above exception, another exception occurred: 
Mar 03 23:12:38  
Mar 03 23:12:38 Traceback (most recent call last): 
Mar 03 23:12:38   File "tools/download_mnist.py", line 87, in <module> 
Mar 03 23:12:38     main() 
Mar 03 23:12:38   File "tools/download_mnist.py", line 80, in main 
Mar 03 23:12:38     download(path, url, options.quiet) 
Mar 03 23:12:38   File "tools/download_mnist.py", line 41, in download 
Mar 03 23:12:38     raise RuntimeError('Error downloading resource!') 
Mar 03 23:12:38 RuntimeError: Error downloading resource! 
Mar 03 23:12:39 + cleanup 
Mar 03 23:12:39 + retcode=1 
Mar 03 23:12:39 + set +x 
Mar 03 23:12:39 =================== sccache compilation log =================== 
Mar 03 23:12:39 =========== If your build fails, please take a look at the log above for possible reasons =========== 
Mar 03 23:12:39 Compile requests                 0 
Mar 03 23:12:39 Compile requests executed        0 
Mar 03 23:12:39 Cache hits                       0 
Mar 03 23:12:39 Cache misses                     0 
Mar 03 23:12:39 Cache timeouts                   0

pytorch_linux_xenial_cuda10_1_cudnn7_py3_NO_AVX_NO_AVX2_test (2/2)

Step: "Test" (full log | pattern match details) ❄️

Mar 04 00:23:48 RuntimeError: Error downloading resource!

Mar 04 00:23:48  
Mar 04 00:23:48 During handling of the above exception, another exception occurred: 
Mar 04 00:23:48  
Mar 04 00:23:48 Traceback (most recent call last): 
Mar 04 00:23:48   File "tools/download_mnist.py", line 87, in <module> 
Mar 04 00:23:48     main() 
Mar 04 00:23:48   File "tools/download_mnist.py", line 80, in main 
Mar 04 00:23:48     download(path, url, options.quiet) 
Mar 04 00:23:48   File "tools/download_mnist.py", line 41, in download 
Mar 04 00:23:48     raise RuntimeError('Error downloading resource!') 
Mar 04 00:23:48 RuntimeError: Error downloading resource! 
Mar 04 00:23:48 + cleanup 
Mar 04 00:23:48 + retcode=1 
Mar 04 00:23:48 + set +x 
Mar 04 00:23:48 =================== sccache compilation log =================== 
Mar 04 00:23:48 =========== If your build fails, please take a look at the log above for possible reasons =========== 
Mar 04 00:23:48 Compile requests                19 
Mar 04 00:23:48 Compile requests executed        0 
Mar 04 00:23:48 Cache hits                       0 
Mar 04 00:23:48 Cache misses                     0 
Mar 04 00:23:48 Cache timeouts                   0

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.

This comment has been revised 6 times.

bhosmer

Ooo nice simplification!

I can't see how pushing the catchall/non-catchall distinction down costs us anything important - can't imagine the fatter keys will ever make a noticeable difference, and it's reversible if we ever need to hoist the distinction back out to the top level for some reason.

One question about a std::move removal inline and definitely worth @smessmer putting the magnifying glass to whatever seething nest of implicit conversions I'm probably glossing over

bhosmer · 2020-02-26T17:17:09Z

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

 RegistrationHandleRAII Dispatcher::registerKernel(const OperatorHandle& op, DispatchKey dispatch_key, KernelFunction kernel) {
  // note: this doesn't need the mutex to protect the iterator because write operations on the list keep iterators intact.
-  return op.operatorIterator_->op.registerKernel(std::move(dispatch_key), std::move(kernel));
+  return op.operatorIterator_->op.registerKernel(dispatch_key, std::move(kernel));


No longer need the move bc now implicitly converting to optional? or never needed the move?

We never needed the move, dispatch keys are small

smessmer

Not sure this is a move in the right direction. Shouldn't we start using CompoundOpKey and use that to get rid of catchall kernels entirely instead of merging the catchall kernel API into the regular kernel API?

ezyang · 2020-02-26T19:20:24Z

Not sure this is a move in the right direction. Shouldn't we start using CompoundOpKey and use that to get rid of catchall kernels entirely instead of merging the catchall kernel API into the regular kernel API?

Two reasons:

The highest priority refactor I have in mind right now is enabling schema-less impl-calls (i.e., this is on the schema merge path). I don't really care about this change per se, but it is very difficult to work on the dispatcher because a lot of codepaths are duplicated. This removes some of the duplication, making further refactors easier to do.
There's a yak in the way of CompoundOp, which is coming up with a plan for kernels that use "catch all" in conjunction with Variable. This is something we should do, but it doesn't seem like it is required for the highest priority refactor (described above). I can't fix everything, so I have to pick what I'm going to kill first. I choose not to kill catchall.

bhosmer · 2020-02-26T19:43:44Z

Not sure this is a move in the right direction. Shouldn't we start using CompoundOpKey and use that to get rid of catchall kernels entirely instead of merging the catchall kernel API into the regular kernel API?

If IIUC and CompoundOpKey is a kind of DispatchKey - so the new API would be unitary in the same way as it is here, just trading CompoundOpKey for nullopt - then I'm not sure I see why this isn't a move in the right direction...

smessmer · 2020-02-26T20:20:56Z

Ok I guess I'm fine with this. We'd have to revert the API changes introduced here once we start using CompoundOpKey, but that's not much effort.

bhosmer · 2020-02-26T20:58:22Z

Ok I guess I'm fine with this. We'd have to revert the API changes introduced here once we start using CompoundOpKey, but that's not much effort.

Just to make sure I'm not missing anything though, we'd only have to revert the change to registerKernal to take an optional, right? We wouldn't have to reintroduce the parallel catchall API. (I.e., that work would amortize in the move to the final state)

smessmer · 2020-02-26T21:17:40Z

Ok I guess I'm fine with this. We'd have to revert the API changes introduced here once we start using CompoundOpKey, but that's not much effort.

Just to make sure I'm not missing anything though, we'd only have to revert the change to registerKernal to take an optional, right? We wouldn't have to reintroduce the parallel catchall API. (I.e., that work would amortize in the move to the final state)

yes that's correct

…p on optional<DispatchKey>" Then, nullopt denotes catch all, whereas everything else is specific to a DispatchKey. I can delete the second copy of methods when I do this. This refactor should be pushed all the way to the frontend but I am doing it one step at a time. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D20125163](https://our.internmc.facebook.com/intern/diff/D20125163) [ghstack-poisoned]

…al<DispatchKey> Then, nullopt denotes catch all, whereas everything else is specific to a DispatchKey. I can delete the second copy of methods when I do this. This refactor should be pushed all the way to the frontend but I am doing it one step at a time. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 6d2a7ae Pull Request resolved: #33817

ezyang · 2020-02-26T21:23:17Z

I updated this to plumb the change all the way through, turns out it was only a few more lines.

…p on optional<DispatchKey>" Then, nullopt denotes catch all, whereas everything else is specific to a DispatchKey. I can delete the second copy of methods when I do this. This refactor should be pushed all the way to the frontend but I am doing it one step at a time. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D20125163](https://our.internmc.facebook.com/intern/diff/D20125163) [ghstack-poisoned]

…al<DispatchKey> (pytorch#33817) Summary: Pull Request resolved: pytorch#33817 Then, nullopt denotes catch all, whereas everything else is specific to a DispatchKey. I can delete the second copy of methods when I do this. This refactor should be pushed all the way to the frontend but I am doing it one step at a time. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Test Plan: Imported from OSS Differential Revision: D20125163 Pulled By: ezyang fbshipit-source-id: 026075a4bab81b0bd88b07f0800f6e6bbeb2166a

facebook-github-bot · 2020-03-05T09:14:14Z

@ezyang merged this pull request in 22506ae.

ezyang requested review from smessmer and bhosmer February 26, 2020 17:08

bhosmer approved these changes Feb 26, 2020

View reviewed changes

smessmer requested changes Feb 26, 2020

View reviewed changes

smessmer approved these changes Feb 26, 2020

View reviewed changes

ezyang mentioned this pull request Feb 26, 2020

Beef up documentation on Dispatcher.h, reorder methods for clarity. #33838

Closed

ezyang mentioned this pull request Mar 3, 2020

Delete OperatorOptions, absorb AliasAnalysisKind into FunctionSchema. #34160

Closed

facebook-github-bot closed this in 22506ae Mar 4, 2020

ezyang mentioned this pull request Mar 4, 2020

Relax internal invariants of Dispatcher in preparation for schema-less registration #34221

Closed

facebook-github-bot added the merged label Mar 5, 2020

bhosmer mentioned this pull request Mar 10, 2020

Use codegen'ed unboxing wrappers #32521

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reduce code duplication in OperatorEntry by keying hash map on optional<DispatchKey> #33817

Reduce code duplication in OperatorEntry by keying hash map on optional<DispatchKey> #33817

Uh oh!

ezyang commented Feb 26, 2020 •

edited

Loading

Uh oh!

dr-ci bot commented Feb 26, 2020 •

edited

Loading

Uh oh!

bhosmer left a comment

Uh oh!

bhosmer Feb 26, 2020

Uh oh!

smessmer Feb 26, 2020

Uh oh!

smessmer left a comment

Uh oh!

ezyang commented Feb 26, 2020

Uh oh!

bhosmer commented Feb 26, 2020

Uh oh!

smessmer commented Feb 26, 2020

Uh oh!

bhosmer commented Feb 26, 2020 •

edited

Loading

Uh oh!

smessmer commented Feb 26, 2020

Uh oh!

ezyang commented Feb 26, 2020

Uh oh!

facebook-github-bot commented Mar 5, 2020

Uh oh!

Uh oh!

Reduce code duplication in OperatorEntry by keying hash map on optional<DispatchKey> #33817

Reduce code duplication in OperatorEntry by keying hash map on optional<DispatchKey> #33817

Uh oh!

Conversation

ezyang commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 CircleCI build failures summary and remediations

❄️ 2 tentatively flaky failures

pytorch_linux_xenial_cuda10_1_cudnn7_py3_multigpu_test (1/2)

pytorch_linux_xenial_cuda10_1_cudnn7_py3_NO_AVX_NO_AVX2_test (2/2)

Uh oh!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

bhosmer Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

smessmer Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

smessmer left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Feb 26, 2020

Uh oh!

bhosmer commented Feb 26, 2020

Uh oh!

smessmer commented Feb 26, 2020

Uh oh!

bhosmer commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smessmer commented Feb 26, 2020

Uh oh!

ezyang commented Feb 26, 2020

Uh oh!

facebook-github-bot commented Mar 5, 2020

Uh oh!

Uh oh!

ezyang commented Feb 26, 2020 •

edited

Loading

dr-ci bot commented Feb 26, 2020 •

edited

Loading

bhosmer commented Feb 26, 2020 •

edited

Loading