[TF-76][Python Interop] add PythonConvertible conformance to TensorShape #23762

realdoug · 2019-04-03T11:44:58Z

This makes TensorShape conform to PythonConvertible per TF-76.

Couple small notes:

It looks like we can essentially delegate to the implementation in Array
I noticed that there were no tests for Array and Dictionary's existing PythonConvertible conformance so I added those as well.

rxwei · 2019-04-03T18:21:56Z

stdlib/public/TensorFlow/TensorShape.swift

+
+extension TensorShape: PythonConvertible {
+  public var pythonObject: PythonObject {
+    return self.dimensions.pythonObject


Our local convention is to omit self. unless it's necessary.

rxwei · 2019-04-03T18:22:54Z

stdlib/public/TensorFlow/TensorShape.swift

+  }
+
+  public init?(_ pythonObject: PythonObject) {
+    self.init(Array<Int32>(pythonObject)!)


Use short hand type names when possible for array and dictionary types.

This is a failable initializer that allows you to return nil. The use of ! is unsafe.

Suggested change

self.init(Array<Int32>(pythonObject)!)

guard let array = [Int32](pythonObject) else {

return nil

}

self.init(array)

rxwei · 2019-04-03T18:25:53Z

Have you tested something like np.reshape(x, [1, 1])?

rxwei · 2019-04-03T18:26:22Z

Thanks for tackling this!

realdoug · 2019-04-04T01:41:46Z

@rxwei in your example is x a TensorShape instance? If so, that works as expected 👍

Just made the above updates as well.

rxwei · 2019-04-04T03:37:49Z

‘x’ should be a numpy array.

rxwei · 2019-04-04T08:32:44Z

Could you add numpy reshape tests to https://github.com/apple/swift/blob/tensorflow/test/Python/numpy_conversion.swift?

rxwei · 2019-04-04T08:28:48Z

test/Python/python_runtime.swift

  expectEqual(five, Double(5).pythonObject)
+
+  expectEqual(intArray, Array([2, 3]).pythonObject)
+  expectEqual(dict, Dictionary<String, Int32>(["abc": 7]).pythonObject)


Suggested change

expectEqual(dict, Dictionary<String, Int32>(["abc": 7]).pythonObject)

expectEqual(dict, ["abc" : 7].pythonObject)

rxwei · 2019-04-04T08:29:46Z

test/Python/python_runtime.swift

  expectEqual(five, Float(5).pythonObject)
  expectEqual(five, Double(5).pythonObject)
+
+  expectEqual(intArray, Array([2, 3]).pythonObject)


Array literal [2, 3] already resolves to an Array.

Suggested change

expectEqual(intArray, Array([2, 3]).pythonObject)

expectEqual(intArray, [2, 3].pythonObject)

rxwei · 2019-04-04T08:30:49Z

test/Python/python_runtime.swift


+  expectEqual([2, 3], Array(intArray))
+  expectEqual(TensorShape(2, 3), TensorShape(intArray))
+  expectEqual(["abc": 97], Dictionary<String, Int32>(dict))


In the Swift core repo we put a space before :. This is a bit different from tensorflow/swift-apis.

Suggested change

expectEqual(["abc": 97], Dictionary<String, Int32>(dict))

expectEqual(["abc" : 97], Dictionary<String, Int32>(dict))

rxwei · 2019-04-04T08:30:58Z

test/Python/python_runtime.swift

  let half: PythonObject = 0.5
  let string: PythonObject = "abc"
+  let intArray: PythonObject = [2, 3]
+  let dict: PythonObject = ["abc": 97]


Suggested change

let dict: PythonObject = ["abc": 97]

let dict: PythonObject = ["abc" : 97]

rxwei · 2019-04-04T08:31:24Z

test/Python/python_runtime.swift

  let minusOne: PythonObject = -1
  let five: PythonObject = 5
+  let intArray: PythonObject = [2, 3]
+  let dict: PythonObject = ["abc": 7]


Suggested change

let dict: PythonObject = ["abc": 7]

let dict: PythonObject = ["abc" : 7]

realdoug · 2019-04-05T05:09:02Z

@rxwei ok, I updated the syntax in all places in this file for consistency, verified that np.reshape works on the 0.2 release as well as this branch and added some tests for it.

EDIT: added in two more in TensorFlowRuntime/numpy_conversions.swift as well.

rxwei · 2019-04-05T05:15:08Z

test/Python/numpy_conversion.swift

+  }
+
+  let numpyArray1D = np.ones(28)
+  let reshaped3D = np.reshape(numpyArray1D, [2, 7, 2])


Sorry, i meant to ask you to add reshape tests to https://github.com/apple/swift/blob/4e84b9267991c2e88342c83e8698914e335a9e44/test/TensorFlowRuntime/numpy_conversion.swift, where TensorShape is available.

rxwei · 2019-04-05T05:17:32Z

test/TensorFlowRuntime/numpy_conversion.swift

                array)
  }

+  let reshaped = np.reshape(numpyArrayInt32, [2, 3])


Suggested change

let reshaped = np.reshape(numpyArrayInt32, [2, 3])

let reshaped = np.reshape(numpyArrayInt32, [2, 3] as TensorShape)

This is not using TensorShape, which kind of misses the point. Could you make the shape be an explicit TensorShape?

🤦‍♂️ ok i see what you were saying now. How is this look? @rxwei

Looks great.

rxwei · 2019-04-05T05:17:40Z

test/TensorFlowRuntime/numpy_conversion.swift

                tensor.array)
  }

+  let reshaped = np.reshape(numpyArrayInt32, [2, 3])


rxwei · 2019-04-05T05:33:20Z

test/Python/python_runtime.swift


 import Python
 import StdlibUnittest
+import struct TensorFlow.TensorShape


These tests should also move to TensorFlowRuntime/numpy_conversion.swift. Python tests should not depend on the TensorFlow module.

rxwei · 2019-04-05T05:50:52Z

@swift-ci please test tensorflow

rxwei · 2019-04-05T06:09:58Z

@swift-ci please test tensorflow

rxwei · 2019-04-05T11:08:54Z

@swift-ci please clean test tensorflow

rxwei · 2019-04-07T04:42:00Z

@swift-ci please test tensorflow

realdoug · 2019-04-08T00:24:10Z

@rxwei I believe this is a real build failure. I was able to repro on a fresh Linux install after pasting my TensorShape extension onto tensorflow~HEAD. Looking into it but not sure f you have any gut thoughts based on the error message.

EDIT: crux of the issue is that my init(PythonObject) has higher precedence than the variadic init(Int32...) defined above it. Still working on the right fix.

realdoug · 2019-04-09T03:07:44Z

@rxwei Can you rerun CI, please?

I added logic to detect len() and handle the case where pythonObject is a single int.
There is also a bug that I was able to reproduce without any python related code & changing TensorShape(1) to TensorShape([1]) steps around it for now.

rxwei · 2019-04-09T03:39:46Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:39:51Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:39:56Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:40:01Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:40:07Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:41:09Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:41:14Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:47:20Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:47:25Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:47:39Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:47:50Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:49:44Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T03:49:51Z

Ugh... CI is dead.

rxwei · 2019-04-09T03:50:11Z

@swift-ci please test tensorflow

rxwei · 2019-04-09T07:00:41Z

@swift-ci please test tensorflow

dan-zheng · 2019-04-09T15:58:26Z

stdlib/public/TensorFlow/TensorShape.swift

 //
 //===----------------------------------------------------------------------===//

+import Python


Could you please do the following?

Rename stdlib/public/TensorFlow/NumpyConversion.swift to PythonConversion.swift.

Make appropriate changes to the file header so it's not NumPy-specific.

Move import Python and the PythonConvertible logic from this file to PythonConversion.swift.

That would be much appreciated! 🙂

@dan-zheng sure thing: #23891

rxwei · 2019-04-15T20:26:07Z

Hi @realdoug, since this PR changed the semantics of TensorShape(0) to rely on Python, this may have broken some tests in other platforms. I'm going to revert this PR since we are approaching our 0.3 release. Let me know when you have a PR that splits PythonConvertible into two protocols!

…TensorShape (#23762)" This reverts commit da317a5.

…ape (swiftlang#23762) * [TF-76] add TensorShape conformance to PythonConvertible

[TF-76] add TensorShape conformance to PythonConvertible

08e556b

realdoug changed the title ~~[TF-76][TF API] add TensorShape conformance to PythonConvertible~~ [TF-76][Python Interop] add TensorShape conformance to PythonConvertible Apr 3, 2019

realdoug changed the title ~~[TF-76][Python Interop] add TensorShape conformance to PythonConvertible~~ [TF-76][Python Interop] add PythonConvertible conformance to TensorShape Apr 3, 2019

rxwei requested changes Apr 3, 2019

View reviewed changes

dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Apr 4, 2019

resolve suggestions

99205e3

rxwei reviewed Apr 4, 2019

View reviewed changes

syntax change and add numpy reshape tests

4e84b92

add np.reshape test to ShapedArray tests

cd57ddb

rxwei reviewed Apr 5, 2019

View reviewed changes

use TensorShape instead of Array

291d965

rxwei reviewed Apr 5, 2019

View reviewed changes

move all tests to TensorFlowRuntime

38003a2

mv TensorShape test

8faa293

rxwei approved these changes Apr 5, 2019

View reviewed changes

add support for noniterable arg and step around unrelated test failure

89a3f61

rxwei merged commit da317a5 into swiftlang:tensorflow Apr 9, 2019

dan-zheng reviewed Apr 9, 2019

View reviewed changes

realdoug mentioned this pull request Apr 9, 2019

[TF-76] file renames + split PythonConvertible protocol #23891

Merged

rxwei added a commit that referenced this pull request Apr 15, 2019

Revert "[TF-76][Python Interop] add PythonConvertible conformance to …

730b2a4

…TensorShape (#23762)" This reverts commit da317a5.

This was referenced Apr 15, 2019

Revert "[TF-76][Python Interop] add PythonConvertible conformance to TensorShape" #24044

Closed

[TF] Change APIs to use Int instead of Int32. #24012

Merged

rxwei pushed a commit to rxwei/swift that referenced this pull request May 11, 2019

[TF-76][Python Interop] add PythonConvertible conformance to TensorSh…

936b8a5

…ape (swiftlang#23762) * [TF-76] add TensorShape conformance to PythonConvertible

-    self.init(Array<Int32>(pythonObject)!)
+    guard let array = [Int32](pythonObject) else {
+      return nil
+    }
+    self.init(array)

	expectEqual(dict, Dictionary<String, Int32>(["abc": 7]).pythonObject)
	expectEqual(dict, ["abc" : 7].pythonObject)

	expectEqual(intArray, Array([2, 3]).pythonObject)
	expectEqual(intArray, [2, 3].pythonObject)

	expectEqual(["abc": 97], Dictionary<String, Int32>(dict))
	expectEqual(["abc" : 97], Dictionary<String, Int32>(dict))

	let dict: PythonObject = ["abc": 97]
	let dict: PythonObject = ["abc" : 97]

	let dict: PythonObject = ["abc": 7]
	let dict: PythonObject = ["abc" : 7]

	let reshaped = np.reshape(numpyArrayInt32, [2, 3])
	let reshaped = np.reshape(numpyArrayInt32, [2, 3] as TensorShape)

[TF-76][Python Interop] add PythonConvertible conformance to TensorShape #23762

[TF-76][Python Interop] add PythonConvertible conformance to TensorShape #23762

Uh oh!

Conversation

realdoug commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei commented Apr 3, 2019

Uh oh!

rxwei commented Apr 3, 2019

Uh oh!

realdoug commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxwei commented Apr 4, 2019

Uh oh!

rxwei commented Apr 4, 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!

realdoug commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Apr 5, 2019

Uh oh!

rxwei commented Apr 5, 2019

Uh oh!

rxwei commented Apr 5, 2019

Uh oh!

rxwei commented Apr 7, 2019

Uh oh!

realdoug commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realdoug commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

Uh oh!

rxwei commented Apr 9, 2019

realdoug commented Apr 3, 2019 •

edited

Loading

realdoug commented Apr 4, 2019 •

edited

Loading

realdoug commented Apr 5, 2019 •

edited

Loading

realdoug commented Apr 8, 2019 •

edited

Loading

realdoug commented Apr 9, 2019 •

edited

Loading

rxwei commented Apr 9, 2019 •

edited

Loading