Skip to content

Conversation

@dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Oct 8, 2019

Rename the Differentiable protocol to _Differentiable.

This matches upstreaming PR #27511, which adds protocol _Differentiable in a new Differentiable.swift file.

tensorflow branch defines typealias Differentiable = _Differentiable for usability. There should be no user-facing changes, except that some error messages now show '_Differentiable' instead of 'Differentiable'.

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Oct 8, 2019
@dan-zheng dan-zheng requested a review from rxwei October 8, 2019 20:27
@dan-zheng dan-zheng changed the title [Autodiff] Underscore the Differentiable protocol. [AutoDiff] Underscore the Differentiable protocol. Oct 8, 2019
@dan-zheng dan-zheng requested review from bgogul and marcrasi October 8, 2019 21:12
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@jckarter
Copy link
Contributor

jckarter commented Oct 9, 2019

Is the protocol still compiled into the standard library when autodifferentiation is not enabled?

@dan-zheng
Copy link
Contributor Author

Is the protocol still compiled into the standard library when autodifferentiation is not enabled?

Yes - I wonder if that is an issue? Perhaps we can continue discussion on the upstreaming-to-master PR #27511.

Context: we've only merged one upstreaming-to-master PR (#27446), which adds an -enable-experimental-differentiable-programming frontend flag for gating differentiable programming features.

We chose a frontend flag rather than a build-script/CMake flag for easier testing: lit tests for differentiable programming can be run by specifying the flag without recompiling the compiler and standard library.

@jckarter
Copy link
Contributor

jckarter commented Oct 9, 2019

It would be nice to make compiling these things into the standard library conditional until they're officially accepted as standard. We don't want to accidentally release incomplete versions of them as ABI. That doesn't have to happen in this PR specifically, but I think it should be part of the upstreaming work you all are doing.

@jckarter
Copy link
Contributor

jckarter commented Oct 9, 2019

And to be clear, if it's more convenient to you all to have these things compiled in by default, I don't have a strong opinion on whether the build condition enables or disables building autodiff-related standard library code in as the default setting. I'm mostly concerned that we be able to leave them out of our official submissions.

@rxwei
Copy link
Contributor

rxwei commented Oct 9, 2019

We can add a build-script flag and #ifdefs that strip out standard library changes in official releases.

I see the concern with stdlib ABI. The only thing that we currently need in the standard library is the _Differentiable protocol. All other APIs will be added to a separate _Differentiation module, which is also conditionally included based on the build-script flag and gated by the -enable-experimental-differentiable-programming flag.

At some point, we will need to add derivatives for stdlib floating point APIs, which I think can be added with @_alwaysEmitIntoClient so that there's no ABI addition (if I'm understanding this correctly).

I think making this build-script flag enabled by default is more preferable because we can make sure all features are tested on master. Does this sound good to you?

@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@dan-zheng
Copy link
Contributor Author

Per discussion, we will add an -enable-experimental-differentiable-programming build-script/CMake flag to gate standard library additions. TF-885 tracks this.

The flag will be added before #27511 upstreams the _Differentiable protocol to master. The flag will be also added to the tensorflow branch, but needn't block this PR.

@dan-zheng dan-zheng merged commit fb6045c into swiftlang:tensorflow Oct 9, 2019
@dan-zheng dan-zheng deleted the underscore-differentiable branch October 9, 2019 22:10
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.

4 participants