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

WIP: Eager mode #491

Merged
merged 49 commits into from
Mar 26, 2019
Merged

WIP: Eager mode #491

merged 49 commits into from
Mar 26, 2019

Conversation

malmaud
Copy link
Owner

@malmaud malmaud commented Jan 20, 2019

No description provided.

src/eager.jl Outdated
end


mutable struct TensorHandle
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't need to be mutable does it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it does so that it can have a finalizer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might be right there.
Though IIRC you can hook the Finalizer directly to the Ptr

@codecov
Copy link

codecov bot commented Jan 20, 2019

Codecov Report

Merging #491 into master will decrease coverage by 0.47%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
- Coverage   49.04%   48.56%   -0.48%     
==========================================
  Files          79       80       +1     
  Lines        2659     2685      +26     
==========================================
  Hits         1304     1304              
- Misses       1355     1381      +26
Impacted Files Coverage Δ
src/TensorFlow.jl 57.89% <ø> (ø) ⬆️
src/eager.jl 0% <0%> (ø)

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 5b8ff66...ddb4bc7. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 20, 2019

Codecov Report

Merging #491 into master will increase coverage by 4.13%.
The diff coverage is 54.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   49.04%   53.17%   +4.13%     
==========================================
  Files          79       83       +4     
  Lines        2661     2642      -19     
==========================================
+ Hits         1305     1405     +100     
+ Misses       1356     1237     -119
Impacted Files Coverage Δ
src/py.jl 0% <0%> (ø) ⬆️
src/show.jl 16.34% <0%> (+0.15%) ⬆️
src/train.jl 72.29% <0%> (+5%) ⬆️
src/TensorFlow.jl 81.25% <100%> (+23.35%) ⬆️
src/ops/transformations.jl 93.1% <100%> (+8.72%) ⬆️
src/version.jl 92.1% <100%> (+29.36%) ⬆️
src/ops/summaries.jl 100% <100%> (ø) ⬆️
src/generate_ops.jl 15.1% <15.15%> (-36.07%) ⬇️
src/eager.jl 39.58% <39.58%> (ø)
src/keras.jl 69.23% <69.23%> (ø)
... and 31 more

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 7099f05...0492959. Read the comment docs.

src/eager.jl Outdated
end

function setindex!(op::EagerOp, dtype::DataType, attr_name)
@tfcall(:TFE_OpSetAttrType, Cvoid, (Ptr{Cvoid}, Cstring, TF_DataType), op, attr_name, dtype|>jl_to_df_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@tfcall(:TFE_OpSetAttrType, Cvoid, (Ptr{Cvoid}, Cstring, TF_DataType), op, attr_name, dtype|>jl_to_df_type)
tf_dtype = jl_to_df_type(dtype)
@tfcall(:TFE_OpSetAttrType, Cvoid, (Ptr{Cvoid}, Cstring, TF_DataType), op, attr_name, tf_dtype)

src/eager.jl Outdated
end

function set_attr_list(op::EagerOp, attr_name, list::Vector{<:Integer})
list = Int64[Int64(x) for x in list]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
list = Int64[Int64(x) for x in list]
list = Int64.(list)

src/eager.jl Outdated
end

function set_attr_list(op::EagerOp, attr_name, list::Vector{<:AbstractFloat})
list = Float32[Float32(x) for x in list]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
list = Float32[Float32(x) for x in list]
list = Float32.(x)

function set_attr_shape_list(op::EagerOp, attr_name, list::Vector)
dims = Vector{Int64}[]
for shape in list
push!(dims, Int64[shape...])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't thiis an error?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it's right? At least Keno's PR 9325475 touched this code to make it what it is now.

src/eager.jl Outdated
ptr = @tfcall(:TFE_ContextListDevices, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}), ctx, status)
check_status(status)
count = @tfcall(:TF_DeviceListCount, Cint, (Ptr{Cvoid},), ptr)
this = new(ptr, count)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't call new outside of a inner constructor?

@malmaud
Copy link
Owner Author

malmaud commented Jan 31, 2019

Ya all that setattr stuff was jsut copy-pasted from core.jl and blindly tweaked. I suspect a lot of code on this branch will be ugly/buggy as I just try to get a basic system working, at which point I'll worry more about code quality.

@oxinabox
Copy link
Collaborator

OK, I'll stop bugging about it

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.

Ok, cool cool.
A first heavy pass.
I'ld like to take a closer look still at the import ops generation code.
and also the tape/grads code.

Ideally eager mode would not be a state, but just the result of using eager types.

And I think the gradient code really wants rewriting to be more dispatch based. Since we can dispatch on function types.
So it is like https://github.com/JuliaDiff/DiffRules.jl

src/TensorFlow.jl Outdated Show resolved Hide resolved
src/TensorFlow.jl Outdated Show resolved Hide resolved
src/core.jl Outdated Show resolved Hide resolved
src/core.jl Outdated Show resolved Hide resolved
src/core.jl Outdated Show resolved Hide resolved
return new_tape
end

function get_tape()
Copy link
Collaborator

Choose a reason for hiding this comment

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

can and thus should be a 1 line function

Copy link
Owner Author

Choose a reason for hiding this comment

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

not applicable anymore since it's no longer a single expression. although if there's a way to write it more compactly, then we should do it.

src/tape.jl Outdated Show resolved Hide resolved
src/tape.jl Show resolved Hide resolved
src/tape.jl Outdated Show resolved Hide resolved
src/tape.jl Outdated Show resolved Hide resolved
Copy link
Owner Author

@malmaud malmaud left a comment

Choose a reason for hiding this comment

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

OK, I responded to some comments. I'll apply some of your patches now, then push a patch, then pick this up a little later. Thanks again for the great feedback.

src/tape.jl Show resolved Hide resolved
local path
for i in Iterators.countfrom(1)
path = joinpath(log_dir, "events.out.tfevents.$i")
isfile(path) || break
Copy link
Owner Author

Choose a reason for hiding this comment

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

Don't we want is isfile here? The events file is a file, not a directory. Maybe the variable name should be changed from 'path' if that's confusing.

src/show.jl Outdated
@@ -40,12 +40,19 @@ function Base.show(io::IO, t::RawTensor)
end
end

function Base.show(io::IO, t::TensorHandle)
raw_tensor = resolve(t)
jl_array = convert(Array, raw_tensor)
Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

src/ops/summaries.jl Show resolved Hide resolved
src/keras.jl Outdated
lr::TensorHandle
end

SGD(;lr=1e-3)= SGD(convert(TensorHandle, lr))
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is mirroring keras. Let's make on-boarding easy :)

src/keras.jl Show resolved Hide resolved
@malmaud
Copy link
Owner Author

malmaud commented Mar 18, 2019 via email

@malmaud
Copy link
Owner Author

malmaud commented Mar 18, 2019

OK, ready for another review. Not everything is ideal, but as long as it's good enough, I would like to merge soon.

@oxinabox
Copy link
Collaborator

I will try and review this week

@malmaud
Copy link
Owner Author

malmaud commented Mar 26, 2019

I think I'll just merge for now, but I still welcome comments if you get a chance/have the motivation sometime.

@malmaud malmaud merged commit 0635115 into master Mar 26, 2019
@oxinabox
Copy link
Collaborator

Fair, sorry I didn't get back to this

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