Breaking down Layer.swift into a directory #102

Shashi456 · 2019-04-19T09:37:36Z

Shashi456 · 2019-04-19T09:40:25Z

A few choices I took would be to create a separate core file for reshape, flatten, dense and other layers we might have. In the case of swift, we also have a layer protocol defined hence i decided to keep the layer.swift
One last thing is, I've added the Tensorflow disclaimer and imported tensorflow in each of the files, I wasn't very sure if that was mandatory.

I know this review might take a while.

It breaks #76 and #100.

saeta · 2019-04-19T16:09:19Z

Hi @Shashi456 !

Thanks for making this refactoring. I think this is absolutely the right thing to do. However to be fully transparent, we're right in the middle of landing our final fixes for the v0.3 release, and working to get a high quality, stable release. As a result, I think it probably makes most sense to revisit this in ~1.5-2 weeks after we have a solid v0.3 release shipped. Hope that helps provide a bit more context!

All the best,
-Brennan

Shashi456 · 2019-04-19T17:53:59Z

@saeta should i close this pull till then?

I don't mind the delay, I know v0.3 is launching. GOOD LUCK! hope it goes smoothly.

rxwei · 2019-04-19T19:04:33Z

Thanks! An earlier pending PR (#70) has the similar goal of breaking down Layers.swift. I think it would be nice to iterate on that one together since it's already been iterated on for some time. This PR introduces a nice structure in that it groups all layer files under a Layers directory. Perhaps you and @eaplatanios can work together on #70?

Shashi456 · 2019-04-19T19:55:44Z

@rxwei afaik, @eaplatanios told me he was working on creating an operators directory, and that i should work on breaking down on layers.

@eaplatanios any comments?

eaplatanios · 2019-04-19T21:29:00Z

@Shashi456 @rxwei we can definitely bring that in as part of #70. I can port your changes over if you want and we can keep iterating there.

Shashi456 · 2019-04-19T22:14:42Z

@rxwei #70 works on an operators directory, while this is for a layers directory. I don't mind the merging but wouldn't it make the PR too large to handle?

I will put this on hold for a while and look into it after v0.3 is released.

rxwei · 2019-04-19T22:17:33Z

Oh yes, that makes sense to me now! Sorry I had the wrong impression that #70 handled both layers and operators. Ok, let's merge this one for layers next week. Thanks @Shashi456 and @eaplatanios for coordinating!

rxwei

Note: To be merged in the week of 4/22

Thanks for taking on this!

Sources/DeepLearning/Layers/Normalization.swift

Shashi456 · 2019-04-19T22:20:17Z

@rxwei have docker builds been turned off xD?

rxwei · 2019-04-19T22:23:12Z

Investigating!

dan-zheng

LGTM if tests pass!

Sources/DeepLearning/Layers/Layer.swift

Sources/DeepLearning/Layers/Pooling.Swift

Sources/DeepLearning/Layers/Recurrent.Swift

Shashi456 · 2019-04-21T00:34:26Z

@rxwei I will delete the Sources\DeepLearning\Layer.swift file when we are ready to merge. I kept it for now, in case there are any additions to the layer file.

Shashi456 · 2019-04-21T12:10:32Z

@rxwei I think this can be merged now.

Docker hasn't been building for a while.

rxwei · 2019-04-21T19:09:31Z

We'll revisit this next week!

googlebot · 2019-04-26T15:42:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

Shashi456 · 2019-04-28T17:17:27Z

@rxwei can i start working on this again?

rxwei · 2019-04-28T18:50:02Z

Yes. Thanks!

Shashi456 · 2019-05-19T10:22:51Z

@dan-zheng @rxwei So I have reflected changes from the PR's before this, and i have built and test it there are no errors. So if you review this, I will delete the Layer.swift file and finalize this PR.

It's been on hold for a while, and I think this is the right time to bring about this change.

Edit: I tried testing on the nightly and a few errors popped up, most probably because these changes haven't been updated into it. But i will put the errors here. Please do comment if you have any thoughts on them.

/home/paws/swift-apis/Tests/DeepLearningTests/LayerTests.swift:46:21: error: use of unresolved identifier 'MaxPool3D'
        let layer = MaxPool3D<Float>(poolSize: (2, 2, 2), strides: (1, 1, 1), padding: .valid)
                    ^~~~~~~~~
TensorFlow.MaxPool1D:1:15: note: did you mean 'MaxPool1D'?
public struct MaxPool1D<Scalar> : TensorFlow.Layer where Scalar : TensorFlow.TensorFlowFloatingPoint {
              ^
TensorFlow.MaxPool2D:1:15: note: did you mean 'MaxPool2D'?
public struct MaxPool2D<Scalar> : TensorFlow.Layer where Scalar : TensorFlow.TensorFlowFloatingPoint {
              ^
/home/paws/swift-apis/Tests/DeepLearningTests/LayerTests.swift:70:21: error: use of unresolved identifier 'AvgPool3D'
        let layer = AvgPool3D<Float>(poolSize: (2, 4, 5), strides: (1, 1, 1), padding: .valid)
                    ^~~~~~~~~
TensorFlow.AvgPool1D:1:15: note: did you mean 'AvgPool1D'?
public struct AvgPool1D<Scalar> : TensorFlow.Layer where Scalar : TensorFlow.TensorFlowFloatingPoint {
              ^
TensorFlow.AvgPool2D:1:15: note: did you mean 'AvgPool2D'?
public struct AvgPool2D<Scalar> : TensorFlow.Layer where Scalar : TensorFlow.TensorFlowFloatingPoint {
              ^
/home/paws/swift-apis/Tests/DeepLearningTests/LayerTests.swift:158:9: error: global function 'XCTAssertEqual(_:_:_:file:line:)' requires that 'SimpleRNNCell<Float>.State' conform to 'Equatable'
        XCTAssertEqual(output, expected)
        ^
XCTest.XCTAssertEqual:1:13: note: where 'T' = 'SimpleRNNCell<Float>.State'
public func XCTAssertEqual<T>(_ expression1: @autoclosure () throws -> T, _ expression2: @autoclosure () throws -> T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line) where T : Equatable
            ^
/home/paws/swift-apis/Tests/DeepLearningTests/LayerTests.swift:175:65: error: cannot convert value of type 'SimpleRNNCell<Float>.State' to closure result type '_'
        let (𝛁rnn, 𝛁inputs) = pullback(.init(inputs.map { SimpleRNNCell<Float>.State($0) }))

Shashi456 · 2019-05-21T06:19:14Z

@rxwei will you look at it this week?

googlebot · 2019-05-26T17:37:38Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Shashi456 · 2019-05-26T17:38:08Z

@rxwei could you please look at this soon?

rxwei · 2019-05-26T19:06:25Z

Could you merge in the latest changes from master? I’ll look tonight.

Shashi456 · 2019-05-26T19:09:08Z

@rxwei My latest commit includes the changes in #131 and #128. Should i delete layer.swift?

rxwei

Thanks @Shashi456. The changes look good overall. I have one more request other than some inline comments about formatting: I think that Layer.swift should be placed directly under DeepLearning/ instead of being included in Layers because 1) Layer.swift does not define any concrete layer and 2) it may get confused with Core.swift when they reside in the same directory.

Sources/DeepLearning/Layers/Pooling.Swift

Sources/DeepLearning/Layers/Recurrent.Swift

Shashi456 · 2019-05-28T06:04:26Z

@rxwei I will take care of the comments, once I address them, could you trigger a Kokoro build?

rxwei · 2019-05-28T06:12:35Z

@rxwei I will take care of the comments, once I address them, could you trigger a Kokoro build?

Yes.

Shashi456 · 2019-05-28T06:17:12Z

@rxwei one final thing, probably not needed now, but do you want the layertest.swift to be broken down as well? I'm just saying that because ultimately we have a lot of different functionality within layers that needs to be tested. Or would you rather continue with what we have, and think about breaking it down later on?

rxwei · 2019-05-28T06:19:12Z

Yes, it should be broken into a few test files, but I'd suggest doing so only after the ongoing refactoring (e.g. #109).

Sources/DeepLearning/Layers/Pooling.Swift

Shashi456 · 2019-05-28T06:35:24Z

@rxwei I'm sorry for the minor indentation errors. I also just wanted to know, if there is any package like flake8 for python, which takes care of these indentations and style errors within swift? Something like that could make it convenient.

rxwei · 2019-05-28T06:45:23Z

Swift toolchains come with a swift-format tool. However, it is not a finished product -- it doesn't always respect the indentation rules for function parameters. Things are moving and the community has been discussing about an official code formatter.

Shashi456 · 2019-05-28T06:56:33Z

@rxwei Feels good to have no build errors for once xD.

Sources/DeepLearning/Layers/Core.swift

rxwei

LGTM

rxwei · 2019-05-28T07:15:02Z

After this gets merged, we need to update the revision of swift-apis that gets pulled into a toolchain build directory. Could you please start a PR to update https://github.com/apple/swift/blob/cdf3234b9b5f64b5202ba57d51d3b180aafc1b85/utils/update_checkout/update-checkout-config.json#L355 to the latest commit hash in swift-apis?

Shashi456 · 2019-05-28T07:19:10Z

@rxwei I will, but what does e5a288b7ce561fe1b18757d1a440c26eb0c7172d mean and what do i replace it by?

rxwei · 2019-05-28T07:26:01Z

This PR got merged as 087618b, which has hash 087618b57b565cd8cf93a23d51da86e30c102460. The hash in update-checkout-config.json should be replaced with this.

Shashi456 changed the title ~~Breaking down Layers into a directory~~ Breaking down Layers.swift into a directory Apr 19, 2019

Shashi456 changed the title ~~Breaking down Layers.swift into a directory~~ Breaking down Layer.swift into a directory Apr 19, 2019

rxwei added the enhancement New feature or request label Apr 19, 2019

rxwei self-assigned this Apr 19, 2019

rxwei self-requested a review April 19, 2019 22:18

rxwei approved these changes Apr 19, 2019

View reviewed changes

Sources/DeepLearning/Layers/Normalization.swift Show resolved Hide resolved

rxwei requested review from dan-zheng and jekbradbury April 19, 2019 22:24

dan-zheng approved these changes Apr 19, 2019

View reviewed changes

Sources/DeepLearning/Layers/Layer.swift Show resolved Hide resolved

Sources/DeepLearning/Layers/Pooling.Swift Show resolved Hide resolved

Sources/DeepLearning/Layers/Recurrent.Swift Show resolved Hide resolved

Shashi456 force-pushed the dir branch from 305e43a to b8d4b5d Compare April 26, 2019 15:42

Shashi456 force-pushed the dir branch from b8d4b5d to b8dca2d Compare April 29, 2019 13:40

Shashi456 mentioned this pull request May 1, 2019

[TF] TensorFlow/TensorFlowCore Refactoring #109

Merged

rxwei reviewed May 28, 2019

View reviewed changes

Review Changes

398efca

Adding layer to Layer.swift

8e09a8e

rxwei reviewed May 28, 2019

View reviewed changes

Sources/DeepLearning/Layers/Pooling.Swift Outdated Show resolved Hide resolved

Sources/DeepLearning/Layers/Pooling.Swift Outdated Show resolved Hide resolved

Minor errors

59d2aad

rxwei added the kokoro:run label May 28, 2019

kokoro-team removed the kokoro:run label May 28, 2019

rxwei reviewed May 28, 2019

View reviewed changes

Sources/DeepLearning/Layers/Core.swift Outdated Show resolved Hide resolved

removing exported from layer/

0809e02

rxwei added the kokoro:run label May 28, 2019

rxwei approved these changes May 28, 2019

View reviewed changes

kokoro-team removed the kokoro:run label May 28, 2019

rxwei merged commit 087618b into tensorflow:master May 28, 2019

Shashi456 mentioned this pull request May 28, 2019

[TF] Update checkout for 'tensorflow/swift-apis' swiftlang/swift#25087

Merged

Shashi456 deleted the dir branch May 28, 2019 07:39

rxwei mentioned this pull request May 29, 2019

Port tests from apple/swift #136

Merged

Breaking down Layer.swift into a directory #102

Breaking down Layer.swift into a directory #102

Uh oh!

Conversation

Shashi456 commented Apr 19, 2019

Uh oh!

Shashi456 commented Apr 19, 2019

Uh oh!

saeta commented Apr 19, 2019

Uh oh!

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

Uh oh!

Uh oh!

rxwei commented Apr 19, 2019

Uh oh!

Shashi456 commented Apr 19, 2019

Uh oh!

eaplatanios commented Apr 19, 2019

Uh oh!

Shashi456 commented Apr 19, 2019

Uh oh!

rxwei commented Apr 19, 2019

Uh oh!

rxwei left a comment • 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!

Uh oh!

Shashi456 commented Apr 19, 2019

Uh oh!

rxwei commented Apr 19, 2019

Uh oh!

dan-zheng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shashi456 commented Apr 21, 2019

Uh oh!

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

Uh oh!

Uh oh!

rxwei commented Apr 21, 2019

Uh oh!

googlebot commented Apr 26, 2019

Uh oh!

Shashi456 commented Apr 28, 2019

Uh oh!

rxwei commented Apr 28, 2019

Uh oh!

Shashi456 commented May 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shashi456 commented May 21, 2019

Uh oh!

googlebot commented May 26, 2019

Uh oh!

Shashi456 commented May 26, 2019

Uh oh!

rxwei commented May 26, 2019

Uh oh!

Shashi456 commented May 26, 2019

Uh oh!

rxwei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shashi456 commented May 28, 2019

Uh oh!

rxwei commented May 28, 2019

Uh oh!

Shashi456 commented May 28, 2019

Uh oh!

rxwei commented May 28, 2019

Uh oh!

Shashi456 commented Apr 19, 2019 •

edited

Loading

rxwei left a comment •

edited

Loading

Shashi456 commented Apr 21, 2019 •

edited

Loading

Shashi456 commented May 19, 2019 •

edited

Loading

rxwei commented May 28, 2019 •

edited

Loading