-
Notifications
You must be signed in to change notification settings - Fork 214
Add TensorScope #188
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
Closed
Closed
Add TensorScope #188
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
998677d
Start of TensorScope
rnett 591d44d
Finish TensorScope, add test
rnett 34e1429
Javadoc updates
rnett 700f0f8
Add a TensorScope to EagerSession to replicate pointer attachment
rnett 8231407
Make auto-attach optional, use TensorScope in eager session to close …
rnett a9982a4
Test for non-auto-attach scope
rnett d30d7b1
cleanup scopes
rnett 8a7e8be
HasTensors abstraction for resource management of multiple tensors
rnett 38b98f0
Iterable attach methods
rnett bfe6aa5
refactor hierarchy, add release to parent methods
rnett 393b5da
fix NPE
rnett 7a3f365
Javadoc updates
rnett cd6f3d2
New tests, remove eager session tensor closing test
rnett 6bc6fce
remove incorrect test
rnett e2cd366
clarify threading docs
rnett f3ff90a
grammar
rnett 37e0374
formatting
rnett 0837634
Add note about different scopes
rnett 555b13b
Add option to not require parent to Tensor and HasTensors
rnett 4319a65
Adjust API to be more explicit, add release
rnett c13c8db
Make constructor package private, use static methods.
rnett 7ebb447
format
rnett 49ac26e
fixes
rnett b4b3ed4
remove extra closed tracking
rnett 775d8bd
New tests, make static methods build on eachother
rnett 8032310
Convert to scope-passing style
rnett dacd0c3
Add no-output run method
rnett 9ceeefd
Doc updates
rnett 344fe2b
Fix framework
rnett 9a846dc
Set TF_Tensor size properly, update docs
rnett a10043e
remove unneeded synchronizeds
rnett eabf063
Don't register memory for view tensors (i.e. from eager operations)
rnett cf8b24b
Initial Rebase fixes
rnett a50d06c
More framework fixes
rnett 46645ee
Rebase fixes
rnett File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
457 changes: 227 additions & 230 deletions
457
tensorflow-core/tensorflow-core-api/src/gen/annotations/org/tensorflow/op/Ops.java
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the scope is going to end up by convention as the last argument. That allows it to be made optional more naturally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we ever want to make it optional though? I'd agree if so, but I never got that impression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's totally possible, for example:
Do you see an advantage in having it as the first argument though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karllessard @Craigacp Any preferences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need to enforce the explicit scoping of tensors resulting from an eager operation like this in all cases. Default behavior would tie these tensors to their eager session, which will release them once closed. Do we want to offer
TensorScope
as a simple utility or as something mandatory? I was under the impression of the former, meaning that we should leave an endpoint accepting noTensorScope
.Related to the order of the arguments in the methods, I don't have a very firm opinion on this but if I need to pick one, I also prefer having optional arguments at the end of the signature rather than the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make most sense to me for an eager session to be a TensorScope, but we'd probably want to have it without attach() and detach(). Maybe we could have a special MutableTensorScope if we really want to have something with attach() and detach()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I'd just use composition instead of inheritance, there's no need to expose it. Eventually I think it would make sense to tie the tensor lifetime to the lifetime of the eager operand, but since we don't have operand lifetimes yet this is fine. I just don't like using EagerSession for scope management very much since it is usually very long lived, but if we have an optional tensor scope version it's fine for now.
For the first vs last argument, I was under the impression that the scope would be mandatory, like I thought Panama's was, but looking at it again I'm not actually sure. If we keep detach (see other reply), I'm not sure what benefit optional scopes would really provide, other than removing some boilerplate. If we do decide to use optional scopes, I agree about the last argument, I made it first since it is (was?) a required scope, and those live on the left, at least in Kotlin. It really should be a receiver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK let's try to see how it goes by enforcing a scope for the tensors, which is a better safety net than the GC for sure since these tensors can be quite large, compared to the operands native objects.