Taught tracer to support concrete, intermediate tensors created within tracee #22100

mhong · 2019-01-24T23:44:25Z

These tensors become additional input tensors to feed into the generated trace
graph function. For example, if the tracee is:

func foo(x: TensorPair<Float, Float>) -> Tensor<Float> {
  let y = Tensor<Float>(1.0)
  return x.first + x.second + y
}

Then the generated trace graph function has 3 input tensors: x.first,
x.second, and y, where y is the additional input.

…n tracee. These tensors become additional input tensors to feed into the generated trace graph function. For example, if the tracee is: ```swift func foo(x: TensorPair) -> Tensor { let y = Tensor<1.0> return x.first + x.second + y } ``` Then the generated trace graph function has 3 input tensors: `x.first`, `x.second`, and `y`, where `y` is the additional input.

mhong · 2019-01-24T23:51:42Z

@swift-ci please test tensorflow

mhong · 2019-01-24T23:54:47Z

Also cc @jekbradbury as FYI

rxwei · 2019-01-24T23:55:25Z

let y = Tensor<1.0>

Did you mean Tensor<Float>(1.0)? :)

mhong · 2019-01-25T00:10:35Z

Good catch. :) Fixed.

rxwei · 2019-01-25T00:20:13Z

stdlib/public/TensorFlow/CompilerRuntime.swift

+  /// The number of additional input tensors to the trace graph function,
+  /// created from concrete intermediate tensors in the tracee, such as `y` in
+  /// the code snippet above.
+  var additionalInputTensorCount: Int32 = -1


Is there a reason why the initial value is -1?

This increases the chance of catching bugs (since -1 is never valid), if we forget to set it to some legal value.

I see where you are coming from, however, there's a safer way to do that in Swift -- since you expect additionalInputTensorCount to always be set during class initialization, removing the default value makes you able to catch initialization bugs at compile-time. When you forget to set it in init, Swift gives you a compile-time error saying "member is not initialized", which is significantly better than catching this at runtime.

rxwei · 2019-01-25T00:21:42Z

stdlib/public/TensorFlow/CompilerRuntime.swift

+    // x) is symbolic. The second one for y is concrete, and is computed at
+    // trace creation time, not trace execution time.
+    // Also see the comment block above finalizeAndExecuteTraceFn().
    for (i, output) in outputs.enumerated() {


Suggested change

for (i, output) in outputs.enumerated() {

for (i, output) in outputs.enumerated() where TFE_TensorHandleIsConcrete(output) == 0 {

Nice! Done.

bgogul · 2019-01-25T19:15:28Z

stdlib/public/TensorFlow/CompilerRuntime.swift

+    // trace creation time, not trace execution time.
+    // Also see the comment block above finalizeAndExecuteTraceFn().
    for (i, output) in outputs.enumerated() {
+      if TFE_TensorHandleIsConcrete(output) != 0 {


The TF_GraphToFunction call below sets the number of outputs to symbolicOutputs.count. As far as I understand symbolicOutputs.count <= outputs.count after this PR. Should TF_GraphToFunction have outputs.count for the number of outputs?

I think I understand it now. Basically, the concrete outputs are captured and returned by the _graph function and don't have to be part of the TFFunction outputs.

bgogul · 2019-01-25T19:16:58Z

stdlib/public/TensorFlow/CompilerRuntime.swift

+    // For example, let the tracee be:
+    //   func foo(x: Tensor) -> (Tensor, Tensor) {
+    //     let y = Tensor<Float>(1.0)
+    //     return (x + x, y)


The test added here does not exercise this scenario?

Discussed in person, and TracerTests.testAllBackends("Basic_IntermediateTensors") should cover this case.

mhong · 2019-01-25T21:02:44Z

@swift-ci please test tensorflow

rxwei · 2019-01-27T03:27:55Z

stdlib/public/TensorFlow/CompilerRuntime.swift

+  /// The number of additional input tensors to the trace graph function,
+  /// created from concrete intermediate tensors in the tracee, such as `y` in
+  /// the code snippet above.
+  var additionalInputTensorCount: Int32 = -1


I see where you are coming from, however, there's a safer way to do that in Swift -- since you expect additionalInputTensorCount to always be set during class initialization, removing the default value makes you able to catch initialization bugs at compile-time. When you forget to set it in init, Swift gives you a compile-time error saying "member is not initialized", which is significantly better than catching this at runtime.

rxwei · 2019-01-27T03:30:30Z

stdlib/public/TensorFlow/CompilerRuntime.swift

+      cTraceContext)
+    for i in 0..<additionalInputTensorCount {
+      symbolicInputs.append(TFE_GetInputGraphNodeFromTraceContext(
+                              cTraceContext, UInt32(i)))


Suggested change

cTraceContext, UInt32(i)))

cTraceContext, UInt32(i)))

cTraceContext is indented two spaces after the pervious line on TFE_GetInputGraphNodeFromTraceContext( -- I believe that's the correct indentation?

Oh, I misread.

rxwei · 2019-01-27T03:31:38Z

stdlib/public/TensorFlow/CompilerRuntime.swift

-  debugLog("Running tracee in tracing mode.")
+  // The tracee output can contain a mixture of symbolic and concrete tensors
+  // (see the comment block within TraceContext.finalize()).
+   debugLog("Running tracee in tracing mode.")


Suggested change

debugLog("Running tracee in tracing mode.")

debugLog("Running tracee in tracing mode.")

Good catch, thanks!

rxwei · 2019-01-27T22:39:57Z

func foo(x: TensorPair) -> Tensor {
 let y = Tensor<Float>(1.0)
 return x.first + x.second + y
}

Nit: TensorPair is a generic type. It'd be clearer to show a program that actually compiles in the PR description.

func foo(x: TensorPair<Float, Float>) -> Tensor<Float> {
  let y = Tensor<Float>(1.0)
  return x.first + x.second + y
}

mhong · 2019-01-28T22:35:20Z

@rxwei: On additionalInputTensorCount, it's not initialized at init(), but at finalize(). So unfortunately we cannot rely on the "definitive initialization" feature in Swift.

rxwei · 2019-01-28T22:39:09Z

Ah, I misread the diff. I thought it was in init(). This SGTM!

mhong · 2019-01-28T22:39:13Z

Thanks all for the review! Submitting this patch. Happy to address any follow-up comments.

mhong requested review from bgogul, pschuh and rxwei January 24, 2019 23:54

Some polishing work and typo fixes.

01c927f

rxwei reviewed Jan 25, 2019

View reviewed changes

bgogul reviewed Jan 25, 2019

View reviewed changes

Addressed feedback.

111e249

rxwei approved these changes Jan 27, 2019

View reviewed changes

bgogul approved these changes Jan 28, 2019

View reviewed changes

mhong merged commit e4d1eba into swiftlang:tensorflow Jan 28, 2019

mhong deleted the trace3_intermediate_tensors branch January 28, 2019 22:39

	for (i, output) in outputs.enumerated() {
	for (i, output) in outputs.enumerated() where TFE_TensorHandleIsConcrete(output) == 0 {

	debugLog("Running tracee in tracing mode.")
	debugLog("Running tracee in tracing mode.")

Taught tracer to support concrete, intermediate tensors created within tracee #22100

Taught tracer to support concrete, intermediate tensors created within tracee #22100

Uh oh!

Conversation

mhong commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhong commented Jan 24, 2019

Uh oh!

mhong commented Jan 24, 2019

Uh oh!

rxwei commented Jan 24, 2019

Uh oh!

mhong commented Jan 25, 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!

rxwei Jan 25, 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!

mhong commented Jan 25, 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!

rxwei commented Jan 27, 2019

Uh oh!

mhong commented Jan 28, 2019

Uh oh!

rxwei commented Jan 28, 2019

Uh oh!

mhong commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhong commented Jan 24, 2019 •

edited

Loading

rxwei Jan 25, 2019 •

edited

Loading