Fix _empty_per_channel_affine_quantized to be less hacky #26243

dzhulgakov · 2019-09-14T23:44:22Z

Stack from ghstack:

Allow per-channel QTensor accept any floating type for scales #26676 Allow per-channel QTensor accept any floating type for scales
Per-channel quantized tensor to have only a single axis #26675 Per-channel quantized tensor to have only a single axis
Fix _empty_per_channel_affine_quantized to be less hacky #26243 Fix _empty_per_channel_affine_quantized to be less hacky

This is an attempt to fix _empty_per_channel_affine_quantized to be more sane. It's a factory function that nevertheless receives a Tensor argument and it throws the codegen off course.

Before people did a hacky workaround of appending _like to the function name to trick codegen, it also required non-natural argument order.

This PR explicitly allows to override the 'category' of the function to make codegen do the right thing. Now name and the argument order (in C++) make more sense.

Gist with changes to codegen: https://gist.github.com/dzhulgakov/87050fcf7527730bea6483600f8c4897 (from ./tools/git_add_generated_dirs.sh)

Differential Revision: D17443221

ghstack-source-id: f579191e44b8176b7271d10e421d05edc156b35a Pull Request resolved: #26243

ghstack-source-id: 3d06e3059d294332506ed7d943ebb18f395e6ffe Pull Request resolved: #26243

jamesr66a · 2019-09-18T18:59:56Z

Can you post a diff of the codegen'ed files before and after this change?

jamesr66a · 2019-09-18T18:58:47Z

aten/src/ATen/native/README.md

@@ -99,7 +99,9 @@ signature.
 Functions with no tensor inputs are called *factory functions*, and
 are handled specially by code generation.  If your function is behaving
 differently than another example, check first and see if one is a
-factory while another is not.
+factory while another is not. In some rare cases, factory function might have a
+tensor argument. In this case it'd be marked with 'category_override: factory'


it'd be marked -> mark it

jerryzh168 · 2019-09-18T21:50:13Z

aten/src/ATen/native/quantized/QTensor.cpp

@@ -142,10 +142,10 @@ Tensor per_channel_affine_qtensor_cpu(
    const Tensor& scales,
    const Tensor& zero_points,
    IntArrayRef axis) {
-  Tensor dst = at::_empty_per_channel_affine_quantized_like(
+  Tensor dst = at::_empty_per_channel_affine_quantized(


what should be the order of arguments?
should we put size in the end so it is closer to options?

I think sizes should be first - similarly to how we do it for at::full or at::rand

raghuramank100

Looks good to me, will let folks more familiar with this part of the code approve.

gchanan

ya I'd like to see the codegen changes.

I think the original issue is we didn't know how to dispatch these things. Because we usually dispatch based on the tensor argument but in this case, I'm not even sure what we were dispatching on. I also think @ezyang's recent multi dispatch changes might have affected this, because I know he was changing how "new" functions worked.

dzhulgakov · 2019-09-19T02:09:07Z

Gist with changes to codegen: https://gist.github.com/dzhulgakov/87050fcf7527730bea6483600f8c4897 (from ./tools/git_add_generated_dirs.sh)

The changes look good to me. The only material changes are in VariableType argument passing and they seem correct (before we were inheriting device and scalar type from quantizer arguments and it doesn't make sense.

ezyang · 2019-09-19T14:40:48Z

The conflict with my changes looks very mild and I can help resolve the changes if I land before this one. (Or if you land before me, I can fix it in my patch, but I'm landing right now and hope it will work this time ;)

This is an attempt to fix _empty_per_channel_affine_quantized to be more sane. It's a factory function that nevertheless receives a Tensor argument and it throws the codegen off course. Before people did a hacky workaround of appending _like to the function name to trick codegen, it also required non-natural argument order. This PR explicitly allows to override the 'category' of the function to make codegen do the right thing. Now name and the argument order (in C++) make more sense. Gist with changes to codegen: https://gist.github.com/dzhulgakov/87050fcf7527730bea6483600f8c4897 (from ./tools/git_add_generated_dirs.sh) Differential Revision: [D17443221](https://our.internmc.facebook.com/intern/diff/D17443221) [ghstack-poisoned]

ghstack-source-id: 979c1bf9e97751ac04e3bfce45127bf897e1f8e8 Pull Request resolved: #26243

This is an attempt to fix _empty_per_channel_affine_quantized to be more sane. It's a factory function that nevertheless receives a Tensor argument and it throws the codegen off course. Before people did a hacky workaround of appending _like to the function name to trick codegen, it also required non-natural argument order. This PR explicitly allows to override the 'category' of the function to make codegen do the right thing. Now name and the argument order (in C++) make more sense. Gist with changes to codegen: https://gist.github.com/dzhulgakov/87050fcf7527730bea6483600f8c4897 (from ./tools/git_add_generated_dirs.sh) Differential Revision: [D17443221](https://our.internmc.facebook.com/intern/diff/D17443221) [ghstack-poisoned]

ghstack-source-id: 799516945cff58c03cc8f0b213a5cc40ae93e8a1 Pull Request resolved: #26243

gchanan · 2019-09-23T19:56:19Z

tools/autograd/gen_python_functions.py

@@ -593,12 +593,13 @@ def get_python_binding_arguments(declaration):
                # produce a compile-time error that is obvious
                has_tensor_return = True

-        is_like_function = name.endswith('_like')
+        category = declaration['category_override']


nit: I would expect that "category" is the correct category at the end of this block, might be worth calling this category_override, which is how it's used.

gchanan · 2019-09-23T20:09:47Z

aten/src/ATen/native/native_functions.yaml

@@ -991,7 +991,9 @@
  dispatch:
    QuantizedCPU: empty_affine_quantized_cpu

- func: _empty_per_channel_affine_quantized_like(Tensor self, Tensor zero_points, int[] size, int[] axis, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None, MemoryFormat? memory_format=contiguous_format) -> Tensor
+# it's a factory function receiving a tensor argument, thus overriding explicitly
+- func: _empty_per_channel_affine_quantized(int[] size, *, Tensor scales, Tensor zero_points, int[] axis, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None, MemoryFormat? memory_format=contiguous_format) -> Tensor


I think this is all a pre-existing issue with TensorOptions, but I don't see how the dispatch makes sense here.

For example, if I don't specify a dtype, I won't try to dispatch to QuantizedCPU and presumably I'll just get some crappy error message about _empty_per_channel_affine_quantized not being implemented for CPU backend.

But really...dtype is not optional. So maybe we don't want to do anything until @izdeby breaks apart TensorOptions, but it seems reasonable to provide a CPU overload and to give a real error message here.

Good idea, let me fix the error message.Btw, it applies to other qtensor factory functions too

This is an attempt to fix _empty_per_channel_affine_quantized to be more sane. It's a factory function that nevertheless receives a Tensor argument and it throws the codegen off course. Before people did a hacky workaround of appending _like to the function name to trick codegen, it also required non-natural argument order. This PR explicitly allows to override the 'category' of the function to make codegen do the right thing. Now name and the argument order (in C++) make more sense. Gist with changes to codegen: https://gist.github.com/dzhulgakov/87050fcf7527730bea6483600f8c4897 (from ./tools/git_add_generated_dirs.sh) Differential Revision: [D17443221](https://our.internmc.facebook.com/intern/diff/D17443221) [ghstack-poisoned]

Summary: Pull Request resolved: pytorch/pytorch#26243 This is an attempt to fix _empty_per_channel_affine_quantized to be more sane. It's a factory function that nevertheless receives a Tensor argument and it throws the codegen off course. Before people did a hacky workaround of appending _like to the function name to trick codegen, it also required non-natural argument order. This PR explicitly allows to override the 'category' of the function to make codegen do the right thing. Now name and the argument order (in C++) make more sense. Test Plan: Imported from OSS Differential Revision: D17443221 Pulled By: dzhulgakov fbshipit-source-id: c98c1c74473d8cbf637f511d26ceb949d8ae2a1a

facebook-github-bot · 2019-09-24T07:36:46Z

@dzhulgakov merged this pull request in 9aad4d7.

This was referenced Sep 14, 2019

Implement more support for per-channel quantization #26240

Closed

Fold weight permutation inside quantized conv operator #26241

Closed

Fold activation permutation inside quantized conv operator #26242

Closed

dzhulgakov pushed a commit that referenced this pull request Sep 14, 2019

_like fix 1

298941e

ghstack-source-id: f579191e44b8176b7271d10e421d05edc156b35a Pull Request resolved: #26243

dzhulgakov changed the title ~~_like fix 1~~ Fix _empty_per_channel_affine_quantized to be less hacky Sep 14, 2019

dzhulgakov requested review from gchanan, dskhudia, raghuramank100 and jamesr66a September 14, 2019 23:56

dzhulgakov pushed a commit that referenced this pull request Sep 17, 2019

_like fix 1

f01bb70

ghstack-source-id: 3d06e3059d294332506ed7d943ebb18f395e6ffe Pull Request resolved: #26243

dzhulgakov mentioned this pull request Sep 17, 2019

Serialization for per channel qtensor #26339

Closed

jamesr66a reviewed Sep 18, 2019

View reviewed changes

jerryzh168 reviewed Sep 18, 2019

View reviewed changes

raghuramank100 reviewed Sep 18, 2019

View reviewed changes

gchanan reviewed Sep 18, 2019

View reviewed changes

dzhulgakov pushed a commit that referenced this pull request Sep 23, 2019

_like fix 1

e2b0387

ghstack-source-id: 979c1bf9e97751ac04e3bfce45127bf897e1f8e8 Pull Request resolved: #26243

dzhulgakov requested a review from apaszke as a code owner September 23, 2019 16:16

pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 23, 2019

dzhulgakov pushed a commit that referenced this pull request Sep 23, 2019

_like fix 1

7f525e9

ghstack-source-id: 799516945cff58c03cc8f0b213a5cc40ae93e8a1 Pull Request resolved: #26243

gchanan reviewed Sep 23, 2019

View reviewed changes

gchanan approved these changes Sep 23, 2019

View reviewed changes

This was referenced Sep 23, 2019

Per-channel quantized tensor to have only a single axis #26675

Closed

Allow per-channel QTensor accept any floating type for scales #26676

Closed

facebook-github-bot closed this in 9aad4d7 Sep 24, 2019

facebook-github-bot added the merged label Sep 24, 2019

facebook-github-bot deleted the gh/dzhulgakov/4/head branch October 28, 2019 22:08

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix _empty_per_channel_affine_quantized to be less hacky #26243

Fix _empty_per_channel_affine_quantized to be less hacky #26243

dzhulgakov commented Sep 14, 2019 •

edited

jamesr66a commented Sep 18, 2019

jamesr66a Sep 18, 2019

jerryzh168 Sep 18, 2019

dzhulgakov Sep 19, 2019

raghuramank100 left a comment

gchanan left a comment

dzhulgakov commented Sep 19, 2019 •

edited

ezyang commented Sep 19, 2019

gchanan Sep 23, 2019

gchanan Sep 23, 2019

dzhulgakov Sep 23, 2019

facebook-github-bot commented Sep 24, 2019

Fix _empty_per_channel_affine_quantized to be less hacky #26243

Fix _empty_per_channel_affine_quantized to be less hacky #26243

Conversation

dzhulgakov commented Sep 14, 2019 • edited

jamesr66a commented Sep 18, 2019

jamesr66a Sep 18, 2019

Choose a reason for hiding this comment

jerryzh168 Sep 18, 2019

Choose a reason for hiding this comment

dzhulgakov Sep 19, 2019

Choose a reason for hiding this comment

raghuramank100 left a comment

Choose a reason for hiding this comment

gchanan left a comment

Choose a reason for hiding this comment

dzhulgakov commented Sep 19, 2019 • edited

ezyang commented Sep 19, 2019

gchanan Sep 23, 2019

Choose a reason for hiding this comment

gchanan Sep 23, 2019

Choose a reason for hiding this comment

dzhulgakov Sep 23, 2019

Choose a reason for hiding this comment

facebook-github-bot commented Sep 24, 2019

dzhulgakov commented Sep 14, 2019 •

edited

dzhulgakov commented Sep 19, 2019 •

edited