Make addmm meta kernel consistent with mm #84960

antoniojkim · 2022-09-13T22:15:22Z

Change the names of the parameters in the addmm meta kernel to be more consistent with mm. Functionally, the only difference in behaviour should be that addmm meta kernel gets its options from the input tensor instead of from the bias parameter.

Fixes #84930

CC: @ezyang @ngimel @wconstab @ke1337 @glebk-cerebras

pytorch-bot · 2022-09-13T22:15:24Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84960

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 03b8f4f:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ezyang · 2022-09-13T23:50:32Z

No, I don't understand this change. Here's the schema for addmm

- func: addmm(Tensor self, Tensor mat1, Tensor mat2, *, Scalar beta=1, Scalar alpha=1) -> Tensor
  structured_delegate: addmm.out
  variants: function, method
  dispatch:
    SparseCPU: addmm_sparse_dense_cpu
    SparseCUDA: addmm_sparse_dense_cuda
    SparseCsrCPU, SparseCsrCUDA: addmm_sparse_compressed_dense

self is the first arg. Reordering it in the macro doesn't make sense?

antoniojkim · 2022-09-13T23:54:01Z

self is the first arg. Reordering it in the macro doesn't make sense?

oh, I only reordered it to match the mm meta kernel. I didn't need to do this. I can change it back and only make the minimum changes required to resolve the issue

ezyang

OK, better. Can we get a test?

ngimel · 2022-09-14T04:06:58Z

aten/src/ATen/native/LinearAlgebra.cpp

@@ -55,7 +55,7 @@ namespace meta {
      mat1.sizes()[0], "x", mat1.sizes()[1], " and ", mat2.sizes()[0], "x", mat2.sizes()[1], ")"); \
 \
  auto names = at::namedinference::propagate_names_for_addmm(mat1, mat2, self); \
-  set_output_raw_strided(0, {mat1.sizes()[0], mat2.sizes()[1]}, {}, self.options(), names);


should we also check that mat1, mat2 and self dtypes are the same? The actual implementation will error out on the different types.

Seeing as this is an implementation specific detail, and not related to shape inference, is the meta kernel the best place to put this check? I mean, it already errors out in practice which means that the check already exists in the correct places.

the point of the meta kernel is not just for shape inference, it is also used in other contexts such as to run through a sequence of operations and produce the same error messages you'd see in eager without actually running the ops in eager. so the practice is to handle all the shape/dtype inference as well as all the error checking in the meta kernel in a way consistent to eager.

ah, I see. Okay, I can add that check then

oh wait, I can add a check to see if the dtypes of self and mat2 match as they are the bias and weight respectively. However, checking if the input dtype matches the parameters will fail in the case that I originally made the issue for

Cuda addmm behavior is the same, all the dtypes have to match.

The case I'm referring to isn't in any execution mode. Its for custom mixed precision on custom vendor hardware that is only traced via lazy tensors. Hence, the case

torch.float32 torch.float16 torch.float32

should be allowed here and should output type torch.float16 to match the input. The actual casting is done further down our stack, not at the top level.

Ah, then I think I would probably claim your custom AMP is wrong. The two ways I can think of solving this: (1) explicitly generate the casts and fuse then away in your backend compiler, (2) produce an alternative matmul that is type promoting and have the backend target that

okay, we've decided to do with solution (1). In which case, the changes in this PR aren't quite necessary anymore. Do we still want to merge this in (including the added dtype checks)?

Dtype checks would be appreciated!

ezyang · 2022-09-16T17:15:31Z

aten/src/ATen/native/LinearAlgebra.cpp

@@ -48,14 +48,16 @@ namespace detail {
 namespace meta {

 #define ADDMM_META() \
+  TORCH_CHECK(self.scalar_type() == mat2.scalar_type(), "self and mat2 must have the same dtype"); \
+  TORCH_CHECK(mat1.scalar_type() == mat2.scalar_type(), "mat1 and mat2 must have the same dtype"); \


It would be nice to add an ErrorInput to the addmm OpInfo test for this case

I'm not sure what you mean by ErrorInput and I can't find any examples in the test/ directory. Can you please provide an example of what you mean?

ezyang · 2022-09-16T17:16:05Z

test/lazy/test_meta_kernel.py

+
+        try:
+            out_nobias = fc_nobias(input)
+            self.assertTrue(False)  # Should never reach here as the above line should throw exception


More idiomatic is assertRaisesRegex/assertRaises

ezyang · 2022-09-16T17:17:33Z

test/lazy/test_meta_kernel.py

+    def test_addmm_invalid_dtype(self):
+        """Tests that the addmm meta kernel returns the correct output type"""
+        input = torch.ones(2, 2, dtype=torch.float16).to("lazy")
+        self.assertTrue(input.dtype == torch.float16)


more idiomatic: assertEqual

ezyang · 2022-09-19T15:04:41Z

@pytorchbot merge

pytorchmergebot · 2022-09-19T15:06:03Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered without a flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

github-actions · 2022-09-19T15:42:17Z

Hey @antoniojkim.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

ngimel · 2022-09-19T16:07:51Z

test/lazy/test_meta_kernel.py

+
+        fc_nobias = torch.nn.Linear(2, 2, bias=False, dtype=float32).to("lazy")
+
+        with self.assertRaises(Exception):


should this be RuntimeError?

@ezyang

Change the names of the parameters in the `addmm` meta kernel to be more consistent with `mm`. Functionally, the only difference in behaviour should be that `addmm` meta kernel gets its options from the `input` tensor instead of from the `bias` parameter. Fixes #84930 CC: @ezyang @ngimel @wconstab @ke1337 @glebk-cerebras Pull Request resolved: #84960 Approved by: https://github.com/ezyang

antoniojkim requested review from lezcano, nikitaved and IvanYashchuk as code owners September 13, 2022 22:15

pytorch-bot bot added the release notes: linalg_frontend release notes category label Sep 13, 2022

facebook-github-bot added the cla signed label Sep 13, 2022

pytorchbot added the open source label Sep 13, 2022

ezyang approved these changes Sep 14, 2022

View reviewed changes

ngimel reviewed Sep 14, 2022

View reviewed changes

IvanYashchuk removed their request for review September 14, 2022 07:58

lezcano removed their request for review September 14, 2022 08:35

antoniojkim added 4 commits September 16, 2022 07:39

Make addmm meta kernel consistent with mm

12974fd

Address PR comment

19f22ef

Add addmm meta kernel test

90b5243

Add dtype check

4104516

antoniojkim force-pushed the antoniojkim/fix_addmm_meta_kernel branch from c0d85b2 to 4104516 Compare September 16, 2022 14:53

ezyang reviewed Sep 16, 2022

View reviewed changes

ezyang approved these changes Sep 16, 2022

View reviewed changes

Address PR comments

03b8f4f

pytorchmergebot added the Merged label Sep 19, 2022

pytorchmergebot closed this in 6fd8e28 Sep 19, 2022

ngimel reviewed Sep 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make addmm meta kernel consistent with mm #84960

Make addmm meta kernel consistent with mm #84960

antoniojkim commented Sep 13, 2022

pytorch-bot bot commented Sep 13, 2022 •

edited

Loading

ezyang commented Sep 13, 2022

antoniojkim commented Sep 13, 2022

ezyang left a comment

ngimel Sep 14, 2022

antoniojkim Sep 14, 2022

wconstab Sep 14, 2022

antoniojkim Sep 14, 2022

antoniojkim Sep 14, 2022

ngimel Sep 15, 2022

antoniojkim Sep 15, 2022

ezyang Sep 15, 2022

antoniojkim Sep 16, 2022

ezyang Sep 16, 2022

ezyang Sep 16, 2022

antoniojkim Sep 16, 2022

ezyang Sep 16, 2022

ezyang Sep 16, 2022

ezyang commented Sep 19, 2022

pytorchmergebot commented Sep 19, 2022

github-actions bot commented Sep 19, 2022

ngimel Sep 19, 2022


		fc_nobias = torch.nn.Linear(2, 2, bias=False, dtype=float32).to("lazy")

		with self.assertRaises(Exception):

Make addmm meta kernel consistent with mm #84960

Make addmm meta kernel consistent with mm #84960

Conversation

antoniojkim commented Sep 13, 2022

pytorch-bot bot commented Sep 13, 2022 • edited Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/84960

✅ No Failures

ezyang commented Sep 13, 2022

antoniojkim commented Sep 13, 2022

ezyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang commented Sep 19, 2022

pytorchmergebot commented Sep 19, 2022

github-actions bot commented Sep 19, 2022

Choose a reason for hiding this comment

pytorch-bot bot commented Sep 13, 2022 •

edited

Loading