merge interfaces that have an optional scalartype parameter #21088

nairbv · 2019-05-29T20:14:55Z

This change is backwards incompatible in C++ only on mean(), sum(), and prod() interfaces that accepted either of:

Tensor sum(IntArrayRef dim, bool keepdim=false) const;	
Tensor sum(IntArrayRef dim, ScalarType dtype) const;

but now to specify both the dim and dtype will require the keepdim parameter:

Tensor sum(IntArrayRef dim, bool keepdim=false, c10::optional<ScalarType> dtype=c10::nullopt) const;

[xla ci]

nairbv · 2019-05-29T21:06:44Z

@pytorchbot rebase this please

nairbv · 2019-05-30T13:57:32Z

@pytorchbot rebase this please

nairbv · 2019-06-04T15:49:38Z

With latest changes the existing jit tests pass (at least, test_jit.py, when I ran them pre-commit), but it seems to me like I should need to make additional changes to support the dtype arg in torch/csrc/jit/passes/shape_analysis.cpp. If that's correct, I'm not sure what the tests to validate those changes would look like.

wanchaol · 2019-06-04T21:28:58Z

torch/csrc/jit/symbolic_script.cpp

+        def mean_0(self, *, dtype: Optional[int]):
            self_size = self.size()
            self_numel = self.numel()
+            self_scalar_type = self.dtype


this does not correctly unwrap the dtype I believe, self_scalar_type is still an Optional[int], you will probably need to unwrap it using boolean refinement like if is not None

I wasn't trying to unwrap, was just trying to be consistent with how the rest of the code is written. e.g. self.scalar_type() gets converted to self_scalar_type (REPLACEMENTS in def saved_variables in load_derivatives.py).

I think torch.mean() and to() both accept optional. If this is wrong though, how would I write a test that would catch it? It seems like this is working.

https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/native_functions.yaml#L2704 hmmm that's pretty weird, from what i see in the schema, to does not take optional dtype, it also does not have default values, so I am not sure why this is working.

For the mean, it looks like you are changing the API, can you add some tests to test the api in eager and see if Autodiff works in JIT? https://github.com/pytorch/pytorch/blob/master/test/common_methods_invocations.py#L339
The tests right now only have one test case, which does not contains the test case with dtype

ah, I didn't notice this comment. Added a test of ('mean', (S, S, S), (), 'dtype', (True,), (), (), ident, {'dtype': torch.float64}),. Is that what you mean?

passed when I ran locally with:
python test_jit.py TestJitGeneratedAutograd.test_mean_dtype

wanchaol · 2019-06-04T21:58:00Z

torch/csrc/jit/symbolic_script.cpp

+                   *,
+                   dtype: Optional[int]):
            self_size = self.size()
+            self_scalar_type = self.dtype


wanchaol · 2019-06-04T21:59:39Z

torch/csrc/utils/python_arg_parser.cpp

    }
  } else if (type_ == ParameterType::SCALARTYPE) {
-    if (str == "None") {
+    if (str == "None" || str == "c10::nullopt") {


why here is c10::nullopt? python arg parser should only consume None in python rather than nullopt

hmm... I hadn't thought about it in detail since this did fix an error I was getting at this point in the code. The default value is set in c++ though right? If the C++ function is defined like Tensor::mean(Tensor self, c10::optional<ScalarType> dtype = c10::nullopt), and the python function call is tensor.mean(), where should the parser find the conversion from c10::nullopt to None?

Is that just supposed to happen based on how it's defined in native_functions.yaml? Somehow the code didn't work without this change.

nairbv · 2019-06-05T14:15:12Z

torch/csrc/jit/passes/shape_analysis.cpp

    // Additionally:
    //   - First input should be the only tensor input
    //   - has a bool keepdim argument
    static const register_formula_for dim_reduce_ops_with_integer_upcast{


In these register_formula_for entries it looks like the code is explicitly parsing each argument in the registered function. It seems that since I've added a dtype argument, I also need to parse that. I can guess at what the code should look like to do so, but I don't have a failing tests and I'm not sure how to write one.

nairbv · 2019-06-05T14:17:17Z

torch/csrc/jit/passes/shape_analysis.cpp

      node->output()->setType(tp->withSizesStrides(sizes, tp->strides()));
      return true;
-    } else if (node->matches("aten::sum(Tensor self) -> Tensor")) {
+    } else if (node->matches("aten::sum(Tensor self, *, int? dtype) -> Tensor")) {


Similarly here in PropagateCompleteShapeOnNode, it looks like this code explicitly handles each argument. I think I need to add similar code to extract the dtype, but don't have a failing test and I'm not sure how to write one that will reach this code.

https://github.com/pytorch/pytorch/blob/master/test/test_jit.py#L6726 here is an example of how to write shape propagation tests

Jun 04 16:33:39 ERROR: test_backwards (__main__.TestMSNPUTensor) Jun 04 16:33:39 ---------------------------------------------------------------------- Jun 04 16:33:39 Traceback (most recent call last): Jun 04 16:33:39 File "test_cpp_extensions.py", line 670, in test_backwards Jun 04 16:33:39 d = c.sum() Jun 04 16:33:39 RuntimeError: No function registered for schema: sum(Tensor self, ScalarType dtype) -> Tensor

nairbv · 2019-06-05T16:27:19Z

test/cpp_extensions/msnpu_extension.cpp

  register_extension_backend_op(
    Backend::MSNPU,
-    "sum(Tensor self) -> Tensor", &sum_override);
+    "sum(Tensor self, ScalarType dtype) -> Tensor", &sum_override);


Technically this isn't correct since dtype should be optional. That's due to an existing bug, filed here to be fixed separately:

#21416

eellison

Few comments on the shape propagation. You can check-in #18813 while you're working on it as well (and maybe I should land it).

eellison · 2019-06-05T16:30:55Z

test/expect/TestScript.test_optional_scalartype.expect

@@ -0,0 +1,13 @@
+graph(%a : Tensor,
+      %b : Tensor):


Could you find another way to test this other than expect files ? We do not use them. Consider using our filecheck tool as a way of comparing expected textual output

removed the expect test and added FileCheck tests

eellison · 2019-06-05T16:34:01Z

torch/csrc/jit/passes/shape_analysis.cpp

            "aten::log2(Tensor self) -> Tensor",
            "aten::log_sigmoid(Tensor self) -> Tensor",
-            "aten::log_softmax(Tensor self, int dim) -> Tensor",
+            "aten::log_softmax(Tensor self, int dim, *, int? dtype) -> Tensor",


The comment above these sets of ops require that the scalar type of the input is preserved. Now that dtype is an argument it is not longer valid to have these ops in this set.

eellison · 2019-06-05T16:34:22Z

torch/csrc/jit/passes/shape_analysis.cpp

        {
-            "aten::sum(Tensor self) -> Tensor",
-            "aten::prod(Tensor self) -> Tensor",
+            "aten::sum(Tensor self, *, int? dtype) -> Tensor",


eellison · 2019-06-05T16:34:51Z

torch/csrc/jit/passes/shape_analysis.cpp

    static const register_formula_for dim_reduce_ops_with_integer_upcast{
        {
-            "aten::prod(Tensor self, int dim, bool keepdim) -> Tensor",
+            "aten::prod(Tensor self, int dim, bool keepdim, *, int? dtype) -> Tensor",


eellison · 2019-06-05T16:35:18Z

torch/csrc/jit/passes/shape_analysis.cpp

        {
            "aten::logsumexp(Tensor self, int[] dim, bool keepdim) -> Tensor",
-            "aten::mean(Tensor self, int[] dim, bool keepdim) -> Tensor",
+            "aten::mean(Tensor self, int[] dim, bool keepdim, *, int? dtype) -> Tensor",


here as well

eellison · 2019-06-05T16:37:28Z

torch/csrc/jit/passes/shape_analysis.cpp

      node->output()->setType(tp->withSizesStrides(sizes, tp->strides()));
      return true;
-    } else if (node->matches("aten::sum(Tensor self) -> Tensor")) {
+    } else if (node->matches("aten::sum(Tensor self, *, int? dtype) -> Tensor")) {


https://github.com/pytorch/pytorch/blob/master/test/test_jit.py#L6726 here is an example of how to write shape propagation tests

gchanan

approving non-JIT changes.

nairbv · 2019-06-20T19:01:15Z

@eellison / @suo / @wanchaol , can one of you approve the jit-related changes here?

eellison

Looks good to me, thanks for the effort in updating the JIT. I know it's a tricky part of the codebase to navigate.

Please update the test for pairs of dtypes before landing if that comment applies.

eellison · 2019-06-20T19:31:35Z

torch/csrc/jit/passes/shape_analysis.cpp

+
+    // Requirements:
+    //   dims           : preserved
+    //   scalar type    : preserved unless specified.


scalar type : preserved unless specified is inaccurate since integer_upcast is true

right, fixed

eellison · 2019-06-20T19:33:28Z

torch/csrc/jit/passes/shape_analysis.cpp


    // Requirements:
-    //   dims           : preserved if keepdim == false, 1 smaller otherwise
+    //   dims           : preserved if keepdim == false, dim.size() smaller otherwise


is this equivalent to saying preserved if keepdim == false, 0 otherwise

no because dims comes from the dim parameter, not from self. E.g., 3-dimension tensor reduced on two dims gives 1-dim tensor:

>>> torch.ones([3,3,3]).sum([0,1]) tensor([9., 9., 9.])

eellison · 2019-06-20T19:43:03Z

test/test_jit.py

+                    if(not tensor_type.is_floating_point or (dtype is not None and not dtype.is_floating_point)):
+                        if op in ['mean', 'softmax', 'log_softmax']:
+                            continue
+                    return_line = "torch.tensor({}, dtype={}).{}({}dtype={})".format(tensor_data, tensor_type, op, str_args, dtype)


Don't some of the ops have different behavior depending on what the inputs tensors dtype is? If so, don't you need to iterate over all pairs of dtypes for the torch.tensor dtype arg and the op dtype?

I'm not sure what you mean, doesn't it? we nested-loop over dtypes twice, once as dtype and once as tensor_type.

oop yea nvm, you're doing it already, although could tensor_type could iterate over None too?

eellison · 2019-06-20T19:44:01Z

test/test_jit.py

+        self._test_dtype_op_shape(ops, [0, False])
+
+        ops = ['sum', 'mean']
+        self._test_dtype_op_shape(ops, [[0, 1], False], 4)


Nit: maybe use kwargs here so it's easier to tell what the arguments mean

eellison · 2019-06-20T20:26:05Z

@ailzhang @wanchaol take a look at autograd ?

nairbv · 2019-06-21T14:11:59Z

@pytorchbot retest this please

nairbv · 2019-06-21T14:53:11Z

@pytorchbot rebase this please

facebook-github-bot

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

nairbv · 2019-06-21T19:17:56Z

test/common_methods_invocations.py

        ('mean', (), NO_ARGS, 'scalar', (True,)),
        ('mean', (), (0,), 'scalar_dim', (True,), [0]),
        ('mean', (), (0, True,), 'scalar_keepdim_dim', (True,), [0]),
+        ('mean', (S, S, S), (), 'dtype', (True,), (), (), ident, {'dtype': torch.float64}),


adding test with dtype, @wanchaol / @ailzhang

wanchaol

looks good to me. Thanks for adding the test!

Might need to follow up with @ailzhang on the xla test

facebook-github-bot

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

ailzhang · 2019-06-21T21:56:05Z

Yea I will make followup changes on XLA side.

Summary: This change is backwards incompatible in *C++ only* on mean(), sum(), and prod() interfaces that accepted either of: ``` Tensor sum(IntArrayRef dim, bool keepdim=false) const; Tensor sum(IntArrayRef dim, ScalarType dtype) const; ``` but now to specify both the dim and dtype will require the keepdim parameter: ``` Tensor sum(IntArrayRef dim, bool keepdim=false, c10::optional<ScalarType> dtype=c10::nullopt) const; ``` [xla ci] Pull Request resolved: pytorch/pytorch#21088 Reviewed By: ailzhang Differential Revision: D15944971 Pulled By: nairbv fbshipit-source-id: 53473c370813d9470b190aa82764d0aea767ed74

facebook-github-bot · 2019-06-24T16:04:12Z

@nairbv merged this pull request in 142361a.

suo · 2019-06-24T16:27:40Z

tests are failing master with errors like:

Jun 24 15:42:02 	Schema not found for node. File a bug report.
Jun 24 15:42:02 	Node: %21 : Tensor = aten::cumsum(%18, %3, %31) # /opt/conda/lib/python3.6/site-packages/torch/distributions/transforms.py:516:0

Going to revert this PR.

Summary: This is (mostly) the re-application of: #21088 which was reverted due to an issue conflicting with changes in: #22104 Pull Request resolved: #22237 Differential Revision: D16012838 Pulled By: nairbv fbshipit-source-id: 35f4a73c97ab68b4e2648aca96b2176f07b5a883

Summary: This is (mostly) the re-application of: pytorch/pytorch#21088 which was reverted due to an issue conflicting with changes in: pytorch/pytorch#22104 Pull Request resolved: pytorch/pytorch#22237 Differential Revision: D16012838 Pulled By: nairbv fbshipit-source-id: 35f4a73c97ab68b4e2648aca96b2176f07b5a883

merge interfaces that have an optional scalartype parameter

db5fff9

Merge branch 'master' into opt_scalartype_interfaces

a423b31

Merge remote-tracking branch 'origin/master' into HEAD

477b169

pytorchbot and others added 3 commits May 30, 2019 13:57

Merge remote-tracking branch 'origin/master' into HEAD

2b1c819

fix flake8 issues

dacaf64

fix jit tests for optional dtype

f3a5c57

nairbv requested a review from suo June 4, 2019 19:10

ailzhang self-requested a review June 4, 2019 21:25

eellison self-requested a review June 4, 2019 21:26

wanchaol self-requested a review June 4, 2019 21:26

wanchaol reviewed Jun 4, 2019

View reviewed changes

nairbv commented Jun 5, 2019

View reviewed changes

pytorchbot added the module: cpp-extensions Related to torch.utils.cpp_extension label Jun 5, 2019

attempt to fix msnpu_extension test

69e0f09

nairbv mentioned this pull request Jun 5, 2019

cpp extensions should use full schema string #21416

Closed

nairbv commented Jun 5, 2019

View reviewed changes

eellison reviewed Jun 5, 2019

View reviewed changes

fixing shape analysis for opt-dtypes

cc2d0c6

nairbv mentioned this pull request Jun 20, 2019

Consolidate definition of operators/gradients where possible #22024

Open

Merge branch 'master' into opt_scalartype_interfaces

819ba9d

gchanan approved these changes Jun 20, 2019

View reviewed changes

nairbv and others added 2 commits June 20, 2019 14:47

Merge branch 'master' into opt_scalartype_interfaces

9030e5f

commit re-generated tensormethods after merge conflict

3e2577e

eellison approved these changes Jun 20, 2019

View reviewed changes

nairbv added 2 commits June 20, 2019 13:04

fix incorrect comment

a88e5ab

use kwargs

7886a7d

Merge remote-tracking branch 'origin/master' into HEAD

ecde3a2

facebook-github-bot reviewed Jun 21, 2019

View reviewed changes

add a test for mean with dtype to verify jit

8b323af

nairbv commented Jun 21, 2019

View reviewed changes

wanchaol approved these changes Jun 21, 2019

View reviewed changes

facebook-github-bot reviewed Jun 21, 2019

View reviewed changes

facebook-github-bot closed this in 142361a Jun 24, 2019

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

nairbv mentioned this pull request Jun 25, 2019

Re apply optional ScalarType changes #22237

Closed

meganset mentioned this pull request Jun 29, 2019

Nightly libtorch doesn't contain recent refactoring of Generator code #22368

Closed

mruberry added the Merged label Oct 28, 2020

merge interfaces that have an optional scalartype parameter #21088

merge interfaces that have an optional scalartype parameter #21088

Uh oh!

Conversation

nairbv commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nairbv commented May 29, 2019

Uh oh!

nairbv commented May 30, 2019

Uh oh!

nairbv commented Jun 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nairbv Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

nairbv commented Jun 20, 2019

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eellison Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nairbv commented May 29, 2019 •

edited

Loading

nairbv Jun 5, 2019 •

edited

Loading

eellison Jun 20, 2019 •

edited

Loading

suo commented Jun 24, 2019 •

edited

Loading