Break TensorGroup into InputTensorGroup and OutputTensorGroup. #20188

pschuh · 2018-10-31T18:29:37Z

This allows array to conform to InputTensorGroup and participate as an input to a #tfop.

pschuh · 2018-10-31T19:15:34Z

@swift-ci please test tensorflow

marcrasi

Yay!

Could you also add a test that exercises this?

lib/IRGen/IRGenSIL.cpp

marcrasi · 2018-10-31T19:00:06Z

lib/IRGen/IRGenSIL.cpp

-      auto conformance = tfModule->lookupConformance(canType,
-                                                     tensorGroupProto);
+      auto conformance =
+          tfModule->lookupConformance(canType, outputTensorGroupProto);
      assert(conformance && "out type does not conform to TensorGroup");


s/TensorGroup/OutputTensorGroup/

stdlib/public/TensorFlow/CompilerRuntime.swift

stdlib/public/TensorFlow/TensorGroup.swift

pschuh · 2018-10-31T22:49:02Z

@swift-ci please test tensorflow

pschuh · 2018-11-01T00:31:11Z

Updated with fixes.

pschuh · 2018-11-01T00:31:23Z

@swift-ci please test tensorflow

marcrasi · 2018-11-01T00:36:06Z

lib/IRGen/IRGenSIL.cpp

@@ -2060,7 +2063,7 @@ void IRGenSILFunction::visitGraphOperationInst(GraphOperationInst *i) {
  // TODO: We should also handle the following cases:


Ooo, you can remove this whole TODO now!

marcrasi · 2018-11-01T00:37:11Z

test/TensorFlowRuntime/dynamic_compilation_tensor_group.swift

+
+// Test packing dynamic arrays.
+TensorGroupTest.testAllBackends("input, array") {
+  some_tf_op(n: 32);


No semicolon needed

marcrasi · 2018-11-01T00:39:21Z

test/TensorFlowRuntime/dynamic_compilation_tensor_group.swift

+  let ret: TensorHandle<Float> = #tfop("Pack",
+    [arr], T$dtype: Float.tensorFlowDataType, axis: 0)
+  let t: Tensor<Float> = Tensor<Float>(shape: [Int32(n), 3], scalars: arr_exp)
+  expectEqual(Tensor<Float>(handle: ret).array, t.array)


Our convention is to have the expected value first.

Also you can simplify this by making the expected ShapedArray directly with ShapedArray(shape: scalars:) instead of making a tensor and converting it to array.

marcrasi · 2018-11-01T00:40:08Z

test/TensorFlowRuntime/dynamic_compilation_tensor_group.swift

+    arr_exp.insert(contentsOf: [Float(i), 2, 3], at: arr_exp.count)
+  }
+
+  let ret: TensorHandle<Float> = #tfop("Pack",


You should be able to do let ret: Tensor<Float> to avoid having to construct the Tensor yourself. This may require my very latest change that I submitted just about an hour ago.

marcrasi · 2018-11-01T00:41:10Z

stdlib/public/TensorFlow/TensorGroup.swift

-
+/// This protocol is divided from OutputTensorGroup in order for the number of
+/// tensors to be determined at runtime. For example, Array<Tensor<Float>> may
+/// have an unknown number of elements at runtime.


s/runtime/compile time/

marcrasi · 2018-11-01T00:42:34Z

Thanks for the updates! I re-reviewed it.

Is swift-ci still not listening to you? Let me know if you want me to trigger it.

marcrasi · 2018-11-01T00:43:41Z

Also, my conflicting PR is submitted now so expect a few conflicts. Let me know if you want help resolving them. Sorry :)

pschuh · 2018-11-01T00:57:03Z

All good now.

marcrasi · 2018-11-01T01:19:11Z

@swift-ci please test tensorflow

marcrasi · 2018-11-01T01:19:21Z

@swift-ci please test tensorflow

This allows array to conform to InputTensorGroup and participate as an input to a #tfop.

marcrasi · 2018-11-01T01:49:37Z

@swift-ci please test tensorflow

marcrasi · 2018-11-01T01:49:45Z

@swift-ci please test tensorflow

marcrasi · 2018-11-01T01:49:50Z

@swift-ci please test tensorflow

rxwei

Left some comments.

First, I'd like to see some justification on the naming choice. This is user-visible API, meaning that the user has to understand what "input" and "output" are. As far as I can tell, much of this change has been implementation-driven, leaking compiler-specific information to the user space. I believe there are better names, and potentially better solutions to this problem. Why such a hurry in landing this patch?

Second, public API changes are expected to go through a discussion with the owner of API design and Swift readability at a minimum.

rxwei · 2018-11-01T06:50:23Z

stdlib/public/TensorFlow/TensorGroup.swift

+      ptr = ptr!.advanced(by: Int(elem._inputTensorHandleCount))
+    }
+  }
+  public var _inputTensorHandleCount : Int32 { get {


Please reformat this as the following:

public var _inputTensorHandleCount : Int32 { get { ... } }

Would it be even better to do it without the get, like this?

public var _inputTensorHandleCount: Int32 { return ... }

Yes it definitely would. I didn't notice there's no set or there's no special attributes on get.

rxwei · 2018-11-01T06:50:45Z

stdlib/public/TensorFlow/TensorGroup.swift

+      ptr = ptr!.advanced(by: Int(elem._inputTensorHandleCount))
+    }
+  }
+  public var _inputTensorHandleCount : Int32 { get {


Please remove space before :.

rxwei · 2018-11-01T06:51:24Z

stdlib/public/TensorFlow/TensorGroup.swift

    return [Scalar.tensorFlowDataType]
  }

+  public var _inputTensorHandleCount : Int32 {


Please remove the space before :.

rxwei · 2018-11-01T06:51:32Z

stdlib/public/TensorFlow/TensorGroup.swift

    return [Scalar.tensorFlowDataType]
  }

+  public var _inputTensorHandleCount : Int32 {


Please remove the space before :.

rxwei · 2018-11-01T06:51:41Z

stdlib/public/TensorFlow/TensorGroup.swift

    return [TensorDataType(TF_VARIANT)]
  }

+  public var _inputTensorHandleCount : Int32 {


Please remove the space before :.

rxwei · 2018-11-01T06:51:51Z

stdlib/public/TensorFlow/TensorGroup.swift

    return [TensorDataType(TF_RESOURCE)]
  }

+  public var _inputTensorHandleCount : Int32 {


Please remove the space before :.

rxwei · 2018-11-01T06:51:54Z

stdlib/public/TensorFlow/TensorGroup.swift

    return [Scalar.tensorFlowDataType]
  }

+  public var _inputTensorHandleCount : Int32 {


Please remove the space before :.

rxwei · 2018-11-01T06:52:35Z

stdlib/public/TensorFlow/TensorGroup.swift

+///
+/// TODO: Add a derived conformance to TensorGroup so that users don't have
+/// to write the conformance themselves.
+public protocol TensorGroup : InputTensorGroup & OutputTensorGroup {}


This is not idiomatic. Please change this to a typealias instead.

public typealias TensorGroup = InoutTensorGroup & OutputTensorGroup

rxwei · 2018-11-01T06:53:26Z

stdlib/public/TensorFlow/TensorGroup.swift

-  /// The types of the tensor stored properties in this type.
-  static var _typeList: [TensorDataType] { get }
-
+/// This protocol is divided from OutputTensorGroup in order for the number of


This is not valid Swift API doc comment because it's implementation-specific, not something the user is expected to understand. If you wanted it to be seen the library implementer, please use double slashes.

rxwei · 2018-11-01T06:54:38Z

stdlib/public/TensorFlow/TensorGroup.swift

-/// tensor operation as an input list whose elements are the tensor fields of
-/// the type. When a TensorGroup is used as an output, it gets initialized
-/// with its tensor fields set to the tensor operation's tensor outputs.
+/// A protocol for types that can be used as tensor operation inputs. When a


"tensor operation inputs" in an implementation-specific comment. It does not have a meaning to the user.

pschuh requested review from mhong and marcrasi and removed request for mhong October 31, 2018 18:29

marcrasi suggested changes Oct 31, 2018

View reviewed changes

marcrasi mentioned this pull request Oct 31, 2018

Add TensorGroup requirement to Dataset API #20197

Merged

pschuh force-pushed the 3 branch from 309157e to b426758 Compare October 31, 2018 22:42

marcrasi reviewed Nov 1, 2018

View reviewed changes

pschuh force-pushed the 3 branch 2 times, most recently from 610c44c to 4252736 Compare November 1, 2018 00:56

pschuh force-pushed the 3 branch 2 times, most recently from 5871232 to ee3dd73 Compare November 1, 2018 01:13

Break TensorGroup into InputTensorGroup and OutputTensorGroup.

d980da6

This allows array to conform to InputTensorGroup and participate as an input to a #tfop.

pschuh force-pushed the 3 branch from ee3dd73 to d980da6 Compare November 1, 2018 01:45

marcrasi approved these changes Nov 1, 2018

View reviewed changes

pschuh merged commit c605516 into swiftlang:tensorflow Nov 1, 2018

rxwei reviewed Nov 1, 2018

View reviewed changes

		@@ -2060,7 +2063,7 @@ void IRGenSILFunction::visitGraphOperationInst(GraphOperationInst *i) {
		// TODO: We should also handle the following cases:

Break TensorGroup into InputTensorGroup and OutputTensorGroup. #20188

Break TensorGroup into InputTensorGroup and OutputTensorGroup. #20188

Uh oh!

Conversation

pschuh commented Oct 31, 2018

Uh oh!

pschuh commented Oct 31, 2018

Uh oh!

marcrasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pschuh commented Oct 31, 2018

Uh oh!

pschuh commented Nov 1, 2018

Uh oh!

pschuh commented Nov 1, 2018

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!

marcrasi commented Nov 1, 2018

Uh oh!

marcrasi commented Nov 1, 2018

Uh oh!

pschuh commented Nov 1, 2018

Uh oh!

marcrasi commented Nov 1, 2018

Uh oh!

marcrasi commented Nov 1, 2018

Uh oh!

marcrasi commented Nov 1, 2018

Uh oh!

marcrasi commented Nov 1, 2018

Uh oh!

marcrasi commented Nov 1, 2018

Uh oh!

rxwei 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!

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