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

@@ -43,8 +43,8 @@ inline at::ScalarType scalar_type(at::ScalarType 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

@@ -88,7 +88,8 @@ void SparseTensorImpl::set_indices_and_values_unsafe(const Tensor& indices, cons
  AT_CHECK(!indices.is_sparse(), "expected indices to be a dense tensor, but got indices of layout ", indices.layout());
  AT_CHECK(!values.is_sparse(), "expected values to be a dense tensor, but got values of layout ", values.layout());

-  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

@@ -82,7 +85,7 @@ inline LongTensor flatten_indices(const Tensor& indices, IntArrayRef full_size,
      indices_mult_cpu_vec[i] = mult;
      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

@@ -238,8 +242,7 @@ std::ostream& print(std::ostream& stream, const Tensor & tensor_, int64_t linesi
    stream << "size:\n" << tensor_.sizes() << "\n";
    stream << "]";
  } 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

@@ -202,7 +201,7 @@ static Tensor computeLinearIndex(const Tensor & src, TensorList indices) {
    if (indices[i].defined()) {
      // 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

@@ -370,8 +370,10 @@ Tensor dot(const Tensor& self, const Tensor& tensor) {

 Tensor& dot_out(Tensor& result, const Tensor& self, const Tensor& tensor) {
  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

@@ -364,7 +364,7 @@ Tensor ctc_loss(const Tensor& log_probs, const Tensor& targets, IntArrayRef inpu
    }
  }
  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

@@ -12,7 +12,7 @@ Tensor pin_memory(const Tensor& self) {
    AT_ERROR("cannot pin '", self.type().toString(), "' only CPU memory can be pinned");
  }
  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

  // log_probs: input_len x batch_size x num_labels
  // 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

@@ -225,7 +224,7 @@ std::tuple<Tensor, Tensor> ctc_loss_gpu_template(const Tensor& log_probs, const

  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

@@ -4460,7 +4460,7 @@ def test_CTCLoss_lengthchecks_cpu(self):
    def test_CTCLoss_zero_infinity(self):
        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

@@ -83,11 +83,11 @@ Tensor & map2_(Tensor & self, const Tensor & x_, const Tensor & y_, PyObject* fn
  }
  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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce DeprecatedTypeProperties class #17991

Introduce DeprecatedTypeProperties class #17991

li-roy commented Mar 13, 2019 •

edited

Loading

ezyang commented Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

li-roy Mar 29, 2019

ezyang Mar 29, 2019

ezyang Mar 27, 2019

li-roy Mar 29, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

li-roy Mar 29, 2019 •

edited

Loading

ezyang Mar 27, 2019

li-roy Mar 29, 2019

ezyang Mar 27, 2019

li-roy Mar 29, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

ezyang Mar 27, 2019

gchanan commented Mar 29, 2019

li-roy commented Mar 30, 2019

gchanan commented Apr 1, 2019

li-roy commented Apr 1, 2019

ezyang commented Apr 2, 2019

li-roy commented Apr 3, 2019

facebook-github-bot commented Apr 4, 2019

gchanan Apr 2, 2019

pietern Apr 5, 2019

		@@ -43,8 +43,8 @@ inline at::ScalarType scalar_type(at::ScalarType s) {

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

Introduce DeprecatedTypeProperties class #17991

Introduce DeprecatedTypeProperties class #17991

Conversation

li-roy commented Mar 13, 2019 • edited Loading

ezyang commented Mar 27, 2019

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

li-roy Mar 29, 2019 • edited Loading

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

gchanan commented Mar 29, 2019

li-roy commented Mar 30, 2019

gchanan commented Apr 1, 2019

li-roy commented Apr 1, 2019

ezyang commented Apr 2, 2019

li-roy commented Apr 3, 2019

facebook-github-bot commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

li-roy commented Mar 13, 2019 •

edited

Loading

li-roy Mar 29, 2019 •

edited

Loading