Introduce DeprecatedTypeProperties class #17991

li-roy · 2019-03-13T19:43:39Z

Stack:
    :black_circle: #17991 Introduce DeprecatedTypeProperties class  💚
    :white_circle: #17601 Store ScalarType and Backend instead of Type in TensorIterator  💚
    :white_circle: #17786 Pass ScalarType separately from Type in python constructors  💚

changes:
-Breaks bc: Tensor::type() now returns DeprecatedTypeProperties& rather than Type&.
-Added DeprecatedTypeProperties, it serves as a temporary replacement for Type as the return value of Tensor::type(). This contributes to making Type just for dispatch purposes so that we can make it dtype agnostic.
-Tensor::dispatch_type() now returns Type& like Tensor::type() used to do.
-Changed callsites of Tensor::type() appropriately.

Differential Revision: D14443117

Differential Revision: D14443117 Differential Version: 75327518

Differential Revision: D14443117 Differential Version: 75511212

Differential Revision: D14443117 Differential Version: 75608133

Differential Revision: D14443117 Differential Version: 75660088

Differential Revision: D14443117 Differential Version: 75688678

Differential Revision: D14443117 Differential Version: 77000356

Differential Revision: D14443117 Differential Version: 77001327

ezyang · 2019-03-27T21:25:51Z

Can you remind me again how we are solving the BC problem, where the return type name from type() is no longer Type but TypeProperties now?

ezyang · 2019-03-27T21:40:03Z

aten/src/ATen/Dispatch.h

  return s;
 }

 C10_DEPRECATED_MESSAGE("passing at::Type to an AT_DISPATCH macro is deprecated, " \


The deprecation message isn't even accurate anymore lol

ezyang · 2019-03-27T21:43:08Z

aten/src/ATen/SparseTensorImpl.cpp


-  AT_CHECK(values.type().toSparse() == legacyTensorType(*this), "values type must match sparse tensor type");
+  AT_CHECK(values.device().type() == device().type(), "device type of values (", values.device().type(), ") must match device type of device().type()", device().type(), ")");
+  AT_CHECK(values.scalar_type() == typeMetaToScalarType(dtype()), "dtype of values (", values.scalar_type(), ") must match dtype of sparse tensor (", typeMetaToScalarType(dtype()), ")");


I don't understand, why don't you just compare dtype here?

Because the old comparison is comparing Type, which is more than just dtype.

Sure, and that's why you compared device(). But for the second one, why not values.dtype() == dtype()?

ezyang · 2019-03-27T21:45:56Z

aten/src/ATen/SparseTensorUtils.h

      mult *= full_size[i];
    }
-    auto indices_mult_cpu = indices.type().cpu()
+    auto indices_mult_cpu = indices.dispatch_type().cpu()


Do we have a non Type/TypeProperties replacement for tensorFromBlob? Could this site be replaced with it?

ezyang · 2019-03-27T21:47:15Z

aten/src/ATen/core/Formatting.cpp

  } else {
-    Type& cpudouble = tensor_.type().toBackend(Backend::CPU).toScalarType(kDouble);
-    Tensor tensor = tensor_.toType(cpudouble).contiguous();
+    Tensor tensor = tensor_.toType(Backend::CPU, kDouble).contiguous();


More idiomatic is tensor_.to(kCPU, kDouble)

ezyang · 2019-03-27T21:48:13Z

aten/src/ATen/core/TypeProperties.h

+// serves as a replacement return value for Tensor::type(). Previously,
+// Tensor::type() returned Type&, but we are changing Type to not be
+// dtype-specific.
+class TypeProperties {


I can't remember, how much did we bikeshed this name :) cc @gchanan

ezyang · 2019-03-27T21:49:31Z

aten/src/ATen/core/TypeProperties.h

+// dtype-specific.
+class TypeProperties {
+ public:
+  TypeProperties(Backend backend=Backend::Undefined, ScalarType scalar_type=ScalarType::Undefined)


Hot take: we shouldn't make these arguments optional. They should be mandatory, and then we should provide a default constructor that's undefined everything.

ezyang · 2019-03-27T21:50:18Z

aten/src/ATen/core/TypeProperties.h

+    return backend_ != Backend::Undefined && scalar_type_ != ScalarType::Undefined;
+  }
+
+  TypeProperties& operator=(const TypeProperties& other) {


C++ should be able to infer this definition automatically. No need to specify it.

ezyang · 2019-03-27T21:52:39Z

aten/src/ATen/native/Indexing.cpp

      // Cast index to the longType matching src's backend
      // This allows us to support ie indexing a cuda tensor with a cpu tensor
-      Tensor index = (wrapIndexOnce(indices[i], i, src.size(i)) * strides[i]).toType(longType);
+      Tensor index = (wrapIndexOnce(indices[i], i, src.size(i)) * strides[i]).toType(kLong);


just to(kLong)? :)

ezyang · 2019-03-27T21:54:29Z

aten/src/ATen/native/LinearAlgebra.cpp

  result.resize_({});
+  AT_CHECK(result.scalar_type() == self.scalar_type(),
+           "result dtype ", result.scalar_type(), " does not match self dtype ", self.scalar_type());
  // dispatching through type ensures we don't allow mismatched types.


Comment's out of date now? (Or the CHECK above is not needed?)

ezyang · 2019-03-27T21:55:07Z

aten/src/ATen/native/LossCTC.cpp

  }
  if (reduction == Reduction::Mean) {
-    auto target_lengths_t = at::tensor(target_lengths, res.options().device(at::Device(at::Device::Type::CPU)).dtype(kLong)).toType(res.type());
+    auto target_lengths_t = at::tensor(target_lengths, res.options());


Err... really? I'd be chuffed if this is semantics preserving, but it doesn't look obviously so to me?

yeah it's good, there's a lot of instances of redundant stuff in thsi file.

ezyang · 2019-03-27T21:56:51Z

aten/src/ATen/native/Memory.cpp

  }
  auto* allocator = detail::getCUDAHooks().getPinnedMemoryAllocator();
-  auto tensor = self.type().tensorWithAllocator(self.sizes(), self.strides(), allocator);
+  auto tensor = self.dispatch_type().tensorWithAllocator(self.sizes(), self.strides(), allocator);


Another one where it would be nice to stop using the factory function on type.

Can't be done yet, but in the future :)

ezyang · 2019-03-27T21:57:22Z

aten/src/ATen/native/cuda/LossCTC.cu

  // targets [int64]: batch_size x target_length OR sum(target_lengths)
  CheckedFrom c = "ctc_loss_gpu";
  using target_t = typename std::conditional<target_scalar_type == kInt, int, int64_t>::type;
-  auto targets = targets_.toType(log_probs.type().toScalarType(target_scalar_type)); // to log_probs cuda if it isn't there already


Are you sure?!

Yeah, this is the reason for the weird test I changed (that you commented on).

ezyang · 2019-03-27T21:58:26Z

aten/src/ATen/native/cuda/LossCTC.cu

  auto target_lengths_t = at::tensor(target_lengths, targets.options().dtype(kLong));
  auto input_lengths_t = at::tensor(input_lengths, targets.options().dtype(kLong));
-  tg_batch_offsets = tg_batch_offsets.toType(targets.type().toScalarType(kLong));
+  tg_batch_offsets = tg_batch_offsets.toBackend(Backend::CUDA);


Just to(kCUDA) then? Or even just tg_batch_offsets.cuda()

ezyang · 2019-03-27T21:59:43Z

test/test_nn.py

        target_lengths = [60, 25, 20]
        input_lengths = [50, 50, 50]
-        targets = torch.randint(1, 15, (sum(target_lengths),), dtype=torch.int)
+        targets = torch.randint(1, 15, (sum(target_lengths),), dtype=torch.int, device='cuda')


Wow, how did this ever work?!

ezyang · 2019-03-27T22:01:18Z

torch/csrc/utils/tensor_apply.cpp

  if (x_.type() != self.type()) {
    throw TypeError("map2_: expected %s for argument 'x' (got %s)",
-        self.type().toString(), x_.type().toString());
+        self.type().toString().c_str(), x_.type().toString().c_str());


you're a lucky man, temporary lifetime extends for the entirety of the expression lol

Differential Revision: D14443117 Differential Version: 77388718

gchanan · 2019-03-29T22:54:01Z

What was the resolution on the BC issue?

Perhaps if we want to do BC here, we should actually do BC? And rename the internal thing instead? (It seems correct to call the thing DeprecatedTypeProperties in code but we should alias Type to it so we don't break people, and rename the current Type to something else).

li-roy · 2019-03-30T00:36:01Z

Will that help? It still won't return a reference.

Differential Revision: D14443117 Differential Version: 77464130

gchanan · 2019-04-01T14:45:02Z

You can also make it return a reference -- that won't be that difficult, right? You just need a small registration mechanism for which we have a lot of prior art.

Differential Revision: D14443117 Differential Version: 77644959

Differential Revision: D14443117 Differential Version: 77671735

li-roy · 2019-04-01T22:19:45Z

Okay. I'll change it to return a reference. I'll do the renaming change in another PR because there will be a lot of changes.

ezyang · 2019-04-02T18:27:02Z

New changes LGTM. Hope we can kill this soon lol.

Differential Revision: D14443117 Differential Version: 77808612

Differential Revision: D14443117 Differential Version: 77838952

Differential Revision: D14443117 Differential Version: 78000628

li-roy · 2019-04-03T21:04:53Z

@pytorchbot retest this please

Differential Revision: D14443117 Differential Version: 78016243

Differential Revision: D14443117 Differential Version: 78021893

Differential Revision: D14443117 Differential Version: 78048258

Differential Revision: D14443117 Differential Version: 78065247

facebook-github-bot · 2019-04-04T10:12:48Z

This pull request has been merged in c705d9e.

Summary: Pull Request resolved: pytorch/pytorch#17991 changes: -Breaks bc: Tensor::type() now returns DeprecatedTypeProperties& rather than Type&. -Added DeprecatedTypeProperties, it serves as a temporary replacement for Type as the return value of Tensor::type(). This contributes to making Type just for dispatch purposes so that we can make it dtype agnostic. -Tensor::dispatch_type() now returns Type& like Tensor::type() used to do. -Changed callsites of Tensor::type() appropriately. Reviewed By: ezyang Differential Revision: D14443117 fbshipit-source-id: 239ccb7a09626279a71d1a37f8f82e7f57bf7d9e

gchanan · 2019-04-02T19:32:54Z

aten/src/ATen/core/DeprecatedTypeProperties.h

+
+// This class specifies a Backend and a ScalarType. Currently, it primarily
+// serves as a replacement return value for Tensor::type(). Previously,
+// Tensor::type() returned Type&, but we are changing Type to not be


I think this comment is focusing on the wrong thing.

The main point is that type served as two things:

our dispatch mechanism

a 'user-level' API for getting backend/ScalarType and a few other ops

since we want to have freedom to change our dispatch mechanism, we want to keep these separate.

You can then add what you wrote above.

FWIW, we only used it as (2) in c10d, since we care about grouping by type etc.

V1: Initial commit

4cdd5fa

Differential Revision: D14443117 Differential Version: 75327518

li-roy requested review from apaszke, mrshenli and pietern as code owners March 13, 2019 19:43

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

royboy added 5 commits March 14, 2019 18:39

V2: (no description)

9c054b9

Differential Revision: D14443117 Differential Version: 75511212

V3: (no description)

f52f595

Differential Revision: D14443117 Differential Version: 75608133

V5: Merge with parent diff changes

1d9ea98

Differential Revision: D14443117 Differential Version: 75660088

V6: (no description)

8a4e6dc

Differential Revision: D14443117 Differential Version: 75688678

V7: Merge with parent diff changes

a73eb36

Differential Revision: D14443117 Differential Version: 77000356

li-roy changed the title ~~[wip] Introduce TypeProperties class~~ Introduce TypeProperties class Mar 27, 2019

li-roy mentioned this pull request Mar 27, 2019

Use TypeProperties in TensorIterator #18513

Closed

V8: Merge with parent diff changes

a43e749

Differential Revision: D14443117 Differential Version: 77001327

ezyang removed the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 27, 2019

ezyang reviewed Mar 27, 2019

View reviewed changes

V12: Merge with parent diff changes

efff9bf

Differential Revision: D14443117 Differential Version: 77388718

ezyang approved these changes Mar 29, 2019

View reviewed changes

V13: (no description)

30f4401

Differential Revision: D14443117 Differential Version: 77464130

This was referenced Mar 30, 2019

Store ScalarType and Backend instead of Type in TensorIterator #17601

Closed

Pass ScalarType separately from Type in python constructors #17786

Closed

royboy added 2 commits April 1, 2019 12:26

V14: Merge with parent diff changes

41fc197

Differential Revision: D14443117 Differential Version: 77644959

V15: (no description)

b0be616

Differential Revision: D14443117 Differential Version: 77671735

royboy added 3 commits April 2, 2019 12:39

V16: Merge with parent diff changes

295ba92

Differential Revision: D14443117 Differential Version: 77808612

V17: Merge with parent diff changes

cc03462

Differential Revision: D14443117 Differential Version: 77838952

V18: Merge with parent diff changes

9c8e5c8

Differential Revision: D14443117 Differential Version: 78000628

royboy added 4 commits April 3, 2019 14:41

V19: Merge with parent diff changes

6784c7f

Differential Revision: D14443117 Differential Version: 78016243

V20: Merge with parent diff changes

0cb2fbb

Differential Revision: D14443117 Differential Version: 78021893

V21: Merge with parent diff changes

e326d2e

Differential Revision: D14443117 Differential Version: 78048258

V22: Merge with parent diff changes

2fc8a87

Differential Revision: D14443117 Differential Version: 78065247

facebook-github-bot closed this in c705d9e Apr 4, 2019

facebook-github-bot added the merged label Apr 4, 2019

gchanan reviewed Apr 4, 2019

View reviewed changes

Stonesjtu mentioned this pull request Apr 23, 2019

error: class "at::Type" has no member "scalarType" NVIDIA/apex#267

Closed

ptrblck mentioned this pull request Apr 24, 2019

Replace type().ScalarType() with scalar_type() NVIDIA/apex#272

Merged

ngimel mentioned this pull request May 13, 2019

index_put_ no longer accepts indices with non-matching backend #20457

Closed

ezyang deleted the export-D14443117 branch May 30, 2019 15:51

Introduce DeprecatedTypeProperties class #17991

Introduce DeprecatedTypeProperties class #17991

Uh oh!

Conversation

li-roy commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Mar 27, 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!

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!

li-roy Mar 29, 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!

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 commented Mar 29, 2019

Uh oh!

li-roy commented Mar 30, 2019

Uh oh!

gchanan commented Apr 1, 2019

Uh oh!

li-roy commented Apr 1, 2019

Uh oh!

ezyang commented Apr 2, 2019

Uh oh!

li-roy commented Apr 3, 2019

Uh oh!

facebook-github-bot commented Apr 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

li-roy commented Mar 13, 2019 •

edited

Loading

li-roy Mar 29, 2019 •

edited

Loading