Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Adds a mechanism to manually promote constants to inputs in Lazy Tensor. #299

Merged
merged 6 commits into from
Jun 27, 2019

Conversation

bgogul
Copy link
Contributor

@bgogul bgogul commented Jun 26, 2019

Adds a _concreteInputLazyTensor property to the tensor-related types in the library. This provides temporary mechanism to promote constants to inputs manually in trace. Here is an example use case:

import TensorFlow

let a = Tensor<Float>(10.0)
let b = Tensor<Float>(5.0)
let w = a._concreteInputLazyTensor * b
print ("\(w)")

This will result in the following trace for w:

lazyTrace_3(%0: float) -> (%2) {
  %1 = Const[dtype: float, value: 5.0]()
  %2 = Mul[T: float](%0, %1)
} 

Note that the slot for a got promoted to an input. This is added to experiment with the lazy tensor implementation and is mostly intended to be used within the TensorFlow module. If this turns out to be useful, we should provide a cleaner user-visible API.

This PR also adds a helper file for tests related to lazy tensor.

@bgogul bgogul requested review from pschuh and rxwei and removed request for pschuh June 26, 2019 20:10
@bgogul bgogul force-pushed the symbolic_inputs branch from 0c1d1ff to 6328665 Compare June 26, 2019 20:11
Copy link
Contributor

@pschuh pschuh left a comment

Choose a reason for hiding this comment

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

Looks fine. Could you include a motivating example in the description?

@saeta
Copy link
Contributor

saeta commented Jun 26, 2019

+1 for documenting a motivating example. Also, could we start with this being _-prefixed? I'm making a few guesses, but we might want to express these concepts in a slightly different way. I'd like us to carefully think through the API design space here. Does that make sense? Thanks!

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

If this protocol is internal, it need not be underscored. The underscoring rule only applies to implementation-detail types that had to be made public.


extension TensorHandle: _LazyTensorCompatible {
var _lazyTensor: LazyTensor? { handle._lazyTensor }
public var _concreteLazyTensor: TensorHandle { TensorHandle(handle: handle._concreteLazyTensor) }
Copy link
Contributor

Choose a reason for hiding this comment

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

The protocol is internal. Why does this requirement need to be public? Same for other occurrences of public.

@bgogul bgogul changed the title Introduces a LazyTensorCompatible protocol Adds a mechanism to manually promote constants to inputs in Lazy Tensor. Jun 27, 2019

Verified

This commit was signed with the committer’s verified signature.
Ayush1325 Ayush
 - Only add _concreteInputLazyTensor to Tensor types.
 - Move _LazyTensorCompatible protocol to a test helper.
@bgogul bgogul force-pushed the symbolic_inputs branch from 39bf3b2 to 595b31f Compare June 27, 2019 17:11
@bgogul
Copy link
Contributor Author

bgogul commented Jun 27, 2019

@pschuh @saeta @rxwei I scaled back this PR to publicly expose only what I needed. I also underscored it.

I moved the _LazyTensorCompatible protocol to a test helper.I don't think it is useful outside of the tests for the time being. If we have a need, we can reassess and design the API properly.

Copy link
Contributor

@pschuh pschuh left a comment

Choose a reason for hiding this comment

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

Constants in the example don't match up.

@bgogul
Copy link
Contributor Author

bgogul commented Jun 27, 2019

Constants in the example don't match up.

Thanks for flagging it. Fixed now.

@bgogul bgogul merged commit d1decbe into tensorflow:master Jun 27, 2019
@bgogul bgogul deleted the symbolic_inputs branch June 27, 2019 17:39
extension TensorHandle {
/// Returns `Self` that wraps `_concreteInputLazyTensor` of the underlying
/// `_AnyTensorHandle`
public var _concreteInputLazyTensor: TensorHandle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is there a reason this needs to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect that it will only be used within the library. However, I wanted it to be available for experimentation in the user models if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

How and why should a user be expected to use this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to be careful exposing public APIs. I think we need to make sure public APIs have at least some justification of use cases before they are exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case is the one in description where we want to force a constant to be an input to an extracted trace. Ultimately, we may not need this API. However, in the meantime, I want to have this knob while we develop and improve the lazy tensor implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Sorry for missing parts of the PR description! And thanks for explaining :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants