Add two new requirements to TensorArrayProtocol #165

bgogul · 2019-06-01T16:34:17Z

This is one of the first steps in addressing https://bugs.swift.org/browse/TF-542. Specifically, this PR introduces two new requirements to TensorArrayProtocol and TensorGroup:

init(handles: [_AnyTensorHandle])
var _tensorHandles: [_AnyTensorHandle] { get }

This is also necessary for the implementation of LazyTensor.

The Derived conformances for TensorGroup does not address these requirements yet. Therefore, the tests have been changed to conform with TensorGroup explicitly.

rxwei · 2019-06-01T22:48:11Z

Sources/TensorFlow/Core/TensorGroup.swift

    /// starting at address `tensorHandles`.
    init(_owning tensorHandles: UnsafePointer<CTensorHandle>?)
+
+    init(handles: [_AnyTensorHandle])


This change is redundant because TensorGroup inherits from TensorArrayProtocol.

Suggested change

init(handles: [_AnyTensorHandle])

rxwei · 2019-06-01T22:49:20Z

Tests/TensorFlowTests/TensorGroupTests.swift

    // Mutable.
    var mixed: Mixed
+
+    init(simple: Simple, mixed: Mixed) {


This initializer shouldn't be necessary. There's already an implicitly synthesized memberwise initializer.

That is what I thought, but I get the following error, when I don't have these:

[1/2] Compiling TensorFlowTests TensorGroupTests.swift /usr/local/google/home/bgogul/workspace/brain/s4tf/tensorflow-swift-apis/Tests/TensorFlowTests/TensorGroupTests.swift:173:22: error: cannot invoke initializer for type 'Simple' with an argument list of type '(w: Tensor<Float>, b: Tensor<Float>)' let simple = Simple(w: w, b: b) ^ /usr/local/google/home/bgogul/workspace/brain/s4tf/tensorflow-swift-apis/Tests/TensorFlowTests/TensorGroupTests.swift:173:22: note: overloads for 'Simple' exist with these partially matching parameter lists: (_handles: C), (_owning: Optional<UnsafePointer<OpaquePointer>>\ ) let simple = Simple(w: w, b: b) ^ ...

Is it because of the the protocol initializers?

Interesting. Yeah, that's probably why.

rxwei · 2019-06-01T22:51:46Z

Tests/TensorFlowTests/TensorGroupTests.swift

    var t: T
    var u: U
+
+    public init(t: T, u: U) {


Same for this memberwise initializer.

rxwei · 2019-06-01T22:53:56Z

Sources/TensorFlow/Core/TensorGroup.swift

+    var _tensorHandles: [_AnyTensorHandle] { get }

    init(_owning tensorHandles: UnsafePointer<CTensorHandle>?, count: Int)
+    init(handles: [_AnyTensorHandle])


Requiring the argument to be an Array is not ideal, because in a lot of cases we are creating a tensor group from a slice of an array. Converting a slice to an Array creates an unnecessary copy. Instead, I think this should take a generic RandomAccessCollection.

Also, for consistency with other requirements, it's better for the first argument label of this initializer to start with an underscore.

Suggested change

init(handles: [_AnyTensorHandle])

init<C: RandomAccessCollection>(_handles: C) where C.Element == _AnyTensorHandle

Good point. Done.

rxwei · 2019-06-01T22:56:51Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+}
+
+func copyOf<T>(handle: TensorHandle<T>) -> _AnyTensorHandle {


Suggested change

func copyOf<T>(handle: TensorHandle<T>) -> _AnyTensorHandle {

func copy<T>(of handle: TensorHandle<T>) -> _AnyTensorHandle {

Random thoughts (since this is a test, naming doesn't matter at all!):

Swift naming conventions recommend that when a verb/noun phrase that the function represents ("copy of handle") contains a prepositional phrase formed with an argument ("of handle"), make the preposition be an argument label. Also, when an argument label merely repeats the type information of the argument, it ("handle") should be omitted.

What do you think of the following?

Suggested change

func copyOf<T>(handle: TensorHandle<T>) -> _AnyTensorHandle {

func copy<T>(of handle: TensorHandle<T>) -> _AnyTensorHandle {

rxwei · 2019-06-01T23:03:49Z

Sources/TensorFlow/Core/TensorGroup.swift

    /// container still owns them.
    func _unpackTensorHandles(into address: UnsafeMutablePointer<CTensorHandle>?)

    var _tensorHandleCount: Int32 { get }


Should this be changed to Int now?

I think so, but will do so in a subsequent CL. I have updated https://bugs.swift.org/browse/TF-542 to reflect this.

rxwei · 2019-06-01T23:04:43Z

Sources/TensorFlow/Core/TensorHandle.swift

        self.handle = TFETensorHandle(_owning: cTensorHandle)
    }
+
+    @usableFromInline


Drop @usableFromInline?

rxwei · 2019-06-02T03:06:12Z

Sources/TensorFlow/Core/TensorGroup.swift

+
+    public init(handles: [_AnyTensorHandle]) {
+        let size = handles.count / Int(Element._tensorHandleCount)
+        self = Array((0..<size).map {


The map(_:) method already produces an array. No need to call the array initializer.

Suggested change

self = Array((0..<size).map {

self = (0..<size).map {

rxwei · 2019-06-02T03:07:20Z

Sources/TensorFlow/Core/TensorGroup.swift

+    public init(handles: [_AnyTensorHandle]) {
+        let size = handles.count / Int(Element._tensorHandleCount)
+        self = Array((0..<size).map {
+                let start = $0 * Int(Element._tensorHandleCount)


Fix indentation here?

bgogul

Thanks for the review, @rxwei. I addressed your comments. PTAL.

bgogul · 2019-06-03T18:04:11Z

Sources/TensorFlow/Core/TensorGroup.swift

    /// container still owns them.
    func _unpackTensorHandles(into address: UnsafeMutablePointer<CTensorHandle>?)

    var _tensorHandleCount: Int32 { get }


I think so, but will do so in a subsequent CL. I have updated https://bugs.swift.org/browse/TF-542 to reflect this.

bgogul · 2019-06-03T18:04:17Z

Sources/TensorFlow/Core/TensorGroup.swift

+    var _tensorHandles: [_AnyTensorHandle] { get }

    init(_owning tensorHandles: UnsafePointer<CTensorHandle>?, count: Int)
+    init(handles: [_AnyTensorHandle])


Good point. Done.

bgogul · 2019-06-03T18:04:40Z

Sources/TensorFlow/Core/TensorGroup.swift

    /// starting at address `tensorHandles`.
    init(_owning tensorHandles: UnsafePointer<CTensorHandle>?)
+
+    init(handles: [_AnyTensorHandle])


bgogul · 2019-06-03T18:04:47Z

Sources/TensorFlow/Core/TensorGroup.swift

+
+    public init(handles: [_AnyTensorHandle]) {
+        let size = handles.count / Int(Element._tensorHandleCount)
+        self = Array((0..<size).map {


bgogul · 2019-06-03T18:04:52Z

Sources/TensorFlow/Core/TensorGroup.swift

+    public init(handles: [_AnyTensorHandle]) {
+        let size = handles.count / Int(Element._tensorHandleCount)
+        self = Array((0..<size).map {
+                let start = $0 * Int(Element._tensorHandleCount)


bgogul · 2019-06-03T18:04:56Z

Sources/TensorFlow/Core/TensorHandle.swift

        self.handle = TFETensorHandle(_owning: cTensorHandle)
    }
+
+    @usableFromInline


bgogul · 2019-06-03T18:07:09Z

Tests/TensorFlowTests/TensorGroupTests.swift

    // Mutable.
    var mixed: Mixed
+
+    init(simple: Simple, mixed: Mixed) {


That is what I thought, but I get the following error, when I don't have these:

[1/2] Compiling TensorFlowTests TensorGroupTests.swift /usr/local/google/home/bgogul/workspace/brain/s4tf/tensorflow-swift-apis/Tests/TensorFlowTests/TensorGroupTests.swift:173:22: error: cannot invoke initializer for type 'Simple' with an argument list of type '(w: Tensor<Float>, b: Tensor<Float>)' let simple = Simple(w: w, b: b) ^ /usr/local/google/home/bgogul/workspace/brain/s4tf/tensorflow-swift-apis/Tests/TensorFlowTests/TensorGroupTests.swift:173:22: note: overloads for 'Simple' exist with these partially matching parameter lists: (_handles: C), (_owning: Optional<UnsafePointer<OpaquePointer>>\ ) let simple = Simple(w: w, b: b) ^ ...

Is it because of the the protocol initializers?

bgogul · 2019-06-03T18:07:15Z

Tests/TensorFlowTests/TensorGroupTests.swift

    var t: T
    var u: U
+
+    public init(t: T, u: U) {


bgogul · 2019-06-03T18:07:21Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+}
+
+func copyOf<T>(handle: TensorHandle<T>) -> _AnyTensorHandle {


rxwei

Looking great!

rxwei · 2019-06-03T18:18:23Z

Sources/TensorFlow/Core/TensorGroup.swift

    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:18:35Z

Sources/TensorFlow/Core/TensorGroup.swift

    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:18:42Z

Sources/TensorFlow/Core/TensorGroup.swift

    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:18:52Z

Sources/TensorFlow/Core/TensorGroup.swift

    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:18:59Z

Sources/TensorFlow/Core/TensorGroup.swift

    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:30:29Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:30:36Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:30:47Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:30:55Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

rxwei · 2019-06-03T18:31:02Z

Tests/TensorFlowTests/TensorGroupTests.swift

+    }
+
+    public init<C: RandomAccessCollection>(
+        _handles: C) where C.Element == _AnyTensorHandle {


Suggested change

_handles: C) where C.Element == _AnyTensorHandle {

_handles: C

) where C.Element == _AnyTensorHandle {

bgogul · 2019-06-03T19:07:51Z

Oops, just noticed these messages. Here is a PR to fix the style issues: #170

`TensorArrayProtocol` and `TensorGroup` have been [moved](tensorflow/swift-apis#139) to [tensorflow/swift-apis](https://github.com/tensorflow/swift-apis), and their derived conformances tests have been [copied](tensorflow/swift-apis#158) there as well. [tensorflow/swift-apis#165](tensorflow/swift-apis#165) changed the protocol requirements which made `TensorGroup` conformances no longer entirely derivable, but the derived conformances tests in this repo have not been updated, and it's blocking us from updating the checkout hash for tensorflow/swift-apis. This PR removes `TensorGroup` derived conformance tests from apple/swift. We expect future changes to `TensorGroup` derived conformances to thoroughly test against tensorflow/swift-apis.

`TensorArrayProtocol` and `TensorGroup` have been [moved](tensorflow/swift-apis#139) to [tensorflow/swift-apis](https://github.com/tensorflow/swift-apis), and their derived conformances tests have been [copied](tensorflow/swift-apis#158) there as well. [tensorflow/swift-apis#165](tensorflow/swift-apis#165) changed the protocol requirements which made `TensorGroup` conformances no longer entirely derivable, but the derived conformances tests in this repo have not been updated, and it's blocking us from updating the checkout hash for tensorflow/swift-apis. This PR removes `TensorGroup` derived conformance tests from apple/swift. We expect future changes to `TensorGroup` derived conformances to thoroughly test against tensorflow/swift-apis. Unblocks #25235.

bgogul added 3 commits June 1, 2019 09:16

Add _tensorHandles and init(handles: ) to TensorGroup.

99b8d03

Modify tests to make sure the struct conform to TensorGroup.

2eeb847

Change initializers to user the handles: version.

96b0f5a

bgogul requested a review from rxwei June 1, 2019 16:36

rxwei approved these changes Jun 1, 2019

View reviewed changes

rxwei reviewed Jun 2, 2019

View reviewed changes

bgogul added 3 commits June 3, 2019 10:05

Remove init(handles:) from TensorGroup.

e0423fe

Convert init(handles: []) to init<C:RandomAccessCollection>(...)

5a4d789

copyOf -> copy(of:)

75c98e7

bgogul commented Jun 3, 2019

View reviewed changes

bgogul merged commit 738b7a5 into tensorflow:master Jun 3, 2019

bgogul deleted the tensor_group_handle branch June 3, 2019 18:29

rxwei approved these changes Jun 3, 2019

View reviewed changes

bgogul added a commit to bgogul/swift-apis that referenced this pull request Jun 3, 2019

Fix style related issues from tensorflow#165

6821859

bgogul added a commit that referenced this pull request Jun 3, 2019

Fix style related issues from #165 (#170)

f16b750

rxwei changed the title ~~Add two new requirements to TensorGroup and TensorArrayProtocol~~ Add two new requirements to _TensorArrayProtocol Jun 4, 2019

rxwei changed the title ~~Add two new requirements to _TensorArrayProtocol~~ Add two new requirements to TensorArrayProtocol Jun 4, 2019

rxwei mentioned this pull request Jun 4, 2019

[TF] Remove TensorFlow derived conformances tests. swiftlang/swift#25238

Merged

rxwei mentioned this pull request Jun 6, 2019

Updated ResNet to work with 2019-06-04 development snapshot. tensorflow/swift-models#167

Closed

	init(handles: [_AnyTensorHandle])
	init<C: RandomAccessCollection>(_handles: C) where C.Element == _AnyTensorHandle

	func copyOf<T>(handle: TensorHandle<T>) -> _AnyTensorHandle {
	func copy<T>(of handle: TensorHandle<T>) -> _AnyTensorHandle {

	_handles: C) where C.Element == _AnyTensorHandle {
	_handles: C
	) where C.Element == _AnyTensorHandle {

Add two new requirements to TensorArrayProtocol #165

Add two new requirements to TensorArrayProtocol #165

Uh oh!

Conversation

bgogul commented Jun 1, 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!

rxwei Jun 1, 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!

rxwei Jun 1, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

rxwei Jun 1, 2019 •

edited

Loading

rxwei Jun 1, 2019 •

edited

Loading