Skip to content
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

Define Base.close(::Session) #342

Merged
merged 3 commits into from
Oct 24, 2017
Merged

Define Base.close(::Session) #342

merged 3 commits into from
Oct 24, 2017

Conversation

malmaud
Copy link
Owner

@malmaud malmaud commented Oct 24, 2017

No description provided.

@codecov
Copy link

codecov bot commented Oct 24, 2017

Codecov Report

Merging #342 into master will increase coverage by 0.06%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   66.86%   66.93%   +0.06%     
==========================================
  Files          50       50              
  Lines        2958     2967       +9     
==========================================
+ Hits         1978     1986       +8     
- Misses        980      981       +1
Impacted Files Coverage Δ
src/core.jl 82.73% <100%> (+0.11%) ⬆️
src/run.jl 85.05% <75%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d40570...f25ea1e. Read the comment docs.

Copy link
Collaborator

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Are there other things we should be able to close also.
Like Graphs?

src/run.jl Outdated
end

function Base.show(io::IO, err::ClosedSessionError)
print(io, "Tried to call 'run' with a closed TensorFlow session.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is assuming the call-stack.
Right now this can only be thrown by run, true.
But the user will find that out by looking at the call-stack.

I think it should be "The TensorFlow session is closed.")

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point

@malmaud
Copy link
Owner Author

malmaud commented Oct 24, 2017

Closing sessions is important for freeing the GPU resources. The resources attached to the other TensorFlow objects is pretty trivial. I guess it couldn't hurt to add close methods to them sometime though.

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

Successfully merging this pull request may close these issues.

2 participants