[TF] Moved the TensorFlow module to swift-apis. #24452

eaplatanios · 2019-05-02T22:50:16Z

@pschuh @rxwei @saeta This PR moves the entirety of the TensorFlow module to swift-apis. All tests pass locally. :)

eaplatanios · 2019-05-02T23:38:43Z

Quick test on enabling compiler optimizations: only two tests fail:

TensorFlowRuntime/model_autodiff_runtime.swift
TensorFlowRuntime/tensor_autodiff_indirect.swift

Both fail with an error that looks like:

Assertion failed: (!Loc.isInPrologue()), function createAllocRef, file /Users/eaplatanios/Development/GitHub/swift-source-temp/swift/include/swift/SIL/SILBuilder.h, line 433.
Stack dump:
0.	Program arguments: /Users/eaplatanios/Development/GitHub/swift-source-temp/build/Ninja-ReleaseAssert/swift-macosx-x86_64/bin/swift -frontend -c -primary-file /Users/eaplatanios/Development/GitHub/swift-source-temp/swift/test/TensorFlowRuntime/tensor_autodiff_indirect.swift -target x86_64-apple-macosx10.9 -enable-objc-interop -sdk /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -F /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks -F /Users/eaplatanios/Development/GitHub/swift-source-temp/build/Ninja-ReleaseAssert/swift-macosx-x86_64/lib -module-cache-path /Users/eaplatanios/Development/GitHub/swift-source-temp/build/Ninja-ReleaseAssert/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache -swift-version 4 -O -module-name main -o /var/folders/lz/kkv6ld7j5q94nvrrywz5_8w00000gn/T/lit_tmp_jH2NTQ/tensor_autodiff_indirect-1b5506.o
1.	While running pass #2608 SILModuleTransform "MandatoryInlining".
0  swift                    0x0000000106ff55e5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1  swift                    0x0000000106ff45c8 llvm::sys::RunSignalHandlers() + 248
2  swift                    0x0000000106ff5bf2 SignalHandler(int) + 258
3  libsystem_platform.dylib 0x00007fff75194b5d _sigtramp + 29
4  swift                    0x0000000108321df2 cmark_strbuf__initbuf + 132365
5  libsystem_c.dylib        0x00007fff7504e6a6 abort + 127
6  libsystem_c.dylib        0x00007fff7501720d basename_r + 0
7  swift                    0x0000000103e44ac0 swift::SILCloner<swift::SILInlineCloner>::visitAllocRefInst(swift::AllocRefInst*) + 1216
8  swift                    0x0000000103e4388c swift::SILCloner<swift::SILInlineCloner>::visitBlocksDepthFirst(swift::SILBasicBlock*) + 460
9  swift                    0x0000000103e3fca3 swift::SILCloner<swift::SILInlineCloner>::cloneFunctionBody(swift::SILFunction*, swift::SILBasicBlock*, llvm::ArrayRef<swift::SILValue>) + 531
10 swift                    0x0000000103e3e930 swift::SILInlineCloner::cloneInline(llvm::ArrayRef<swift::SILValue>) + 784
11 swift                    0x0000000103e3e517 swift::SILInliner::inlineFunction(swift::SILFunction*, swift::FullApplySite, llvm::ArrayRef<swift::SILValue>) + 247
12 swift                    0x0000000103c5df00 runOnFunctionRecursively(swift::SILOptFunctionBuilder&, swift::SILFunction*, swift::FullApplySite, llvm::DenseSet<swift::SILFunction*, llvm::DenseMapInfo<swift::SILFunction*> >&, llvm::ImmutableSet<swift::SILFunction*, llvm::ImutContainerInfo<swift::SILFunction*> >::Factory&, llvm::ImmutableSet<swift::SILFunction*, llvm::ImutContainerInfo<swift::SILFunction*> >, swift::ClassHierarchyAnalysis*) + 3408
13 swift                    0x0000000103c5d07f (anonymous namespace)::MandatoryInlining::run() + 511
14 swift                    0x0000000103c7b31e swift::SILPassManager::runModulePass(unsigned int) + 734
15 swift                    0x0000000103c7bde4 swift::SILPassManager::execute() + 692
16 swift                    0x00000001031e130b swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 187
17 swift                    0x0000000103c84a14 swift::runSILDiagnosticPasses(swift::SILModule&) + 132
18 swift                    0x000000010308c8a5 performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 10661
19 swift                    0x0000000103088e98 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 3016
20 swift                    0x000000010303aba2 main + 690
21 libdyld.dylib            0x00007fff74fa93d5 start + 1
<unknown>:0: error: unable to execute command: Abort trap: 6
<unknown>:0: error: compile command failed due to signal 6 (use -v to see invocation)

Given that we get these two failures with optimizations, I have not enabled them in this PR, yet, but I think we're very close to being able to support them.

eaplatanios · 2019-05-07T17:36:56Z

@rxwei @pschuh I realized that if we want to split the libraries and be able to compile swift-apis independently, we need to make swift-bindings a Swift PM package and add it to the swift-apis dependencies. Is it ok if I do that?

pschuh · 2019-05-07T17:45:32Z

How would that work? they would be interdependent.

eaplatanios · 2019-05-07T19:23:30Z

@pschuh The new swift-apis does not import anything (as it is itself the TensorFlow module). If we make swift-bindings a SwiftPM package called TensorFlowBindings, for example, it would be added to the swift-apis dependencies and be imported by swift-apis. Then, you'll both be able to build swift-apis independently and test it and you'll also be able to build it as part of apple/swift, pulled by the update checkouts script. I can make the modifications and make sure it works.

eaplatanios · 2019-05-07T19:53:38Z

@pschuh Actually there is an even simpler way. I can introduce a TensorFlowBindings module in the stdlib that just pulls in the bindings and have TensorFlow (which is now located in swift-apis) depend on it (without involving SwiftPM). I just tested it and it works.

@pschuh never mind, you're right they are interdependent. How about we bring swift-bindings inside swift-apis entirely? That would resolve this issue and it would also make them both build and test jointly, without requiring compilation of apple/swift.

rxwei · 2019-05-07T21:28:52Z

Moving to swift-apis is definitely a good direction, but having a huge file in the source code may not be ideal. One option is to require generating wrappers before running swift build, but this may involve a lot of unnecessary dependencies. If I remember correctly, binding generation requires a Python TensorFlow installation.

pschuh · 2019-05-08T02:40:44Z

Yes, binding generation should not happen per-user. I found it is very easy to get inconsistent results from doing this even if you install the dependencies.

We're going to have to have a wrapper makefile or something that builds tensorflow. This could also copy in the current swift-bindings into a sub-directory with gitignore or something. This could work conceptually as though the bindings were generated on-demand. Just have a fallback that if the necessary python dependencies are not available it downloads the bindings from github. Perhaps something along those lines could make things simpler?

eaplatanios · 2019-05-11T18:53:05Z

@rxwei @pschuh I just got back from traveling and can resume with this. Given that the merge seems to have happened, should I now proceed with update these two PRs to reflect all updates for the move to swift-apis?

Regarding the raw bindings, what I did locally on my machine is move the contents of the swift-bindings repo inside a Bindings directory next to Ops, etc. Whenever the bindings need to be updated, the generate_wrappers.py script can be run from within that directory, generating a new RawOpsGenerated.swift file. Otherwise, the DeepLearning/TensorFlow module will be compiled with whatever RawOpsGenerated.swift file is in the Bindings directory. Nothing else is affected, so not much is changing from the current workflow, other than the location of the bindings generation code and sources. Is that acceptable? It works fine on my machine.

pschuh · 2019-05-11T19:07:45Z

Hi Anthony, Sounds good. I have https://github.com/pschuh/swift-apis/tree/experimental-dan-branch where I just did a straight port out of the swift repo to test out some of the workflow things that need to happen since it sounds like we want to build Tensorflow into the toolchain still. Without that, script mode is slightly painful. I'm currently blocked on some bugs, but dan is looking at them. A cmake pre-step that builds tensorflow and downloads the bindings is fine by me. I'm not exactly following what your current plan is, but I'd like to focus on the short-path of getting Tensorflow out of the swift compiler repo. I think in that plan, the /TensorFlow and /DeepLearning get merged into a single repo and download swift-bindings files as a pre-step. We can then organize things better from there.

eaplatanios · 2019-05-11T20:53:54Z

Hey Parker! Regarding your port, for now I am not moving CTensorFlow out. I'm only moving TensorFlow to swift-apis and then also building into into the toolchain. I'm also not using CMake, just rather removing the need for a swift-bindings repository entirely, by making it part of swift-apis too. I'll push changes in the next few hours and it'll be more clear then I believe.

We can also move CTensorFlow out of stdlib, but I believe it'd be better to do this in a separate PR. Btw, great job on merging with master. This seems to already resolve some type inference errors I've been getting.

eaplatanios · 2019-05-11T21:00:56Z

@pschuh You can check out my temporary solution in tensorflow/swift-apis#109. Both that and this PR are now updated. I'm currently compiling and will report here once I'm able to confirm that all tests pass locally.

pschuh · 2019-05-11T21:14:54Z

Ah, very curious. That is another option keeping them separate. An interesting result of that design is we might be able to make a target in the main tensorflow repo that builds a .so file and the swiftmodule that we can include in our toolchain for the base tensorflow build. This would make it so that it would only depend on swift pm.

eaplatanios · 2019-05-11T22:26:54Z

@pschuh All tests pass locally so both this PR and tensorflow/swift-apis#109 should be ready to merge if we decide to do so. Is there anything else blocking us before we can go ahead with this move?

…sorFlow.

Moves all of `stdlib/TensorFlow` from apple/swift. Friend PR: swiftlang/swift#24452.

rxwei · 2019-05-30T09:11:34Z