Skip to content

TensorFlow: remove the unnecessary code in CTensorFlow #29225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

compnerd
Copy link
Member

This removes the local CTensorFlow library which had dead code and a
decent chunk of complexity to import the TensorFlow interfaces. As a
simplifying step towards extracting the TensorFlow module, this
introduces a second copy of the tensorflow-swift-apis code for the
CTensorFlow module.

The user visible parts of this change amount to restructuring the
tensorflow headers and locations. The headers are now installed to
lib/swift/tensorflow and replicate the system header location layout,
of tensorflow/c/.... This allows for re-use of a system-wide
installation of TensorFlow on Linux or a standalone packaged library on
Darwin.

This sets the stage to move towards externalising the build for the
TensorFlow module by removing the dependency on a library out-side of
the directory. The build rules are largely replicating the build
infrastructure which has been constructed in the swift-apis repository.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

@compnerd compnerd added the tensorflow This is for "tensorflow" branch PRs. label Jan 15, 2020
@compnerd
Copy link
Member Author

@swift-ci please test tensorflow

@compnerd compnerd force-pushed the combine-tensorflow-ctensorflow branch 2 times, most recently from f1bcafa to be046c1 Compare January 17, 2020 03:03
@compnerd
Copy link
Member Author

@swift-ci please test tensorflow

@compnerd compnerd force-pushed the combine-tensorflow-ctensorflow branch from be046c1 to fe1a301 Compare January 17, 2020 04:41
@compnerd
Copy link
Member Author

@swift-ci please test tensorflow

@compnerd compnerd force-pushed the combine-tensorflow-ctensorflow branch from fe1a301 to 210ce03 Compare January 17, 2020 05:40
@compnerd
Copy link
Member Author

@swift-ci please test tensorflow

@compnerd
Copy link
Member Author

@swift-ci please clean test tensorflow

This removes the local CTensorFlow library which had dead code and a
decent chunk of complexity to import the TensorFlow interfaces.  As a
simplifying step towards extracting the TensorFlow module, this
introduces a second copy of the tensorflow-swift-apis code for the
CTensorFlow module.

The user visible parts of this change amount to restructuring the
tensorflow headers and locations.  The headers are now installed to
`lib/swift/tensorflow` and replicate the system header location layout,
of `tensorflow/c/...`.  This allows for re-use of a system-wide
installation of TensorFlow on Linux or a standalone packaged library on
Darwin.

This sets the stage to move towards externalising the build for the
TensorFlow module by removing the dependency on a library out-side of
the directory.  The build rules are largely replicating the build
infrastructure which has been constructed in the swift-apis repository.
@compnerd compnerd force-pushed the combine-tensorflow-ctensorflow branch from 210ce03 to 1136b73 Compare January 17, 2020 18:57
@compnerd
Copy link
Member Author

@swift-ci please clean test tensorflow

2 similar comments
@ematejska
Copy link
Contributor

@swift-ci please clean test tensorflow

@compnerd
Copy link
Member Author

@swift-ci please clean test tensorflow

@compnerd compnerd requested a review from dan-zheng January 18, 2020 02:32
@dan-zheng dan-zheng requested a review from pschuh January 18, 2020 02:35
Copy link
Contributor

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user visible parts of this change amount to restructuring the
tensorflow headers and locations. The headers are now installed to
lib/swift/tensorflow and replicate the system header location layout,
of tensorflow/c/.... This allows for re-use of a system-wide
installation of TensorFlow on Linux or a standalone packaged library on
Darwin.

I wonder if this change affects apple/swift tensorflow branch developers (e.g. do README build instructions need to be updated), or no? Perhaps not yet, but the "next steps" sound like they'll require developers to install TensorFlow on their system:

This sets the stage to move towards externalising the build for the
TensorFlow module by removing the dependency on a library out-side of
the directory. The build rules are largely replicating the build
infrastructure which has been constructed in the swift-apis repository.

@dan-zheng
Copy link
Contributor

dan-zheng commented Jan 18, 2020

On the topic of README build instructions: there are two tensorflow-branch specific build-script flags and corresponding CMake variables (--tensorflow-host-include-dir and --tensorflow-host-lib-dir) that you might be interested in reviewing, if you haven't seen them.

In early 2018, @gribozavr recommended adding these two separate flags to be future-facing for TensorFlow cross-compilation support. But in reality, they're not set up properly for cross-compilation, and they're not really used properly.

@compnerd
Copy link
Member Author

compnerd commented Jan 18, 2020

Yeah, think that next steps will be more appropriate for that. Right now, this is just restructuring it internally, and we can use the build of TensorFlow that we are doing. Id like to move away from that and make TensorFlow an external dependency instead. The immediate goals here are to work towards reducing the changes in the tensorflow branch.

I don't think that the options really help the cross-compilation given that the current CMake setup is pretty hostile to cross-compilation. Splitting up the SDK and the toolchain will help clear that up. In the mean time, reducing the complexity (and not supporting cross-compilation via build-script) is probably the fastest way forward to support cross-compilation (in fact, that is how I have android builds setup).

@compnerd compnerd merged commit c728e10 into swiftlang:tensorflow Jan 18, 2020
@compnerd compnerd deleted the combine-tensorflow-ctensorflow branch January 18, 2020 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tensorflow This is for "tensorflow" branch PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants