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

Subtape + Fix Recursion #171

Open
wants to merge 10 commits into
base: subtape
Choose a base branch
from
Open

Conversation

markus7800
Copy link

Hello,

I am not too familiar with the code base, but I updated the subtaping functionality to work for my own project.
Maybe you find these fixes useful.

  • Added subtapes property to TapedFunction
    This is important for task copying to work. We also have to copy the subtapes to continue in the correct instruction in the copied task (e.g. if we want to continue in a subtape).
  • Remove translation optimisation based on constant return types of calls. A call may contain a produce statement but returns a constant. In the old implementation this call would have been optimised.
  • Added a few test cases for subtapes that did not pass in the old implementation, but now do.

Best,
Markus

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

Hey Markus!

First of all, thank you for helping out with this!

Because my familiarity with Libtask is pretty bad, I'm just leaving some comments + trying to understand what's going on here; I'll leave the approval to someone else:)

Am I understanding it correctly that we need to keep the subtapes around because of scenarios such as the following. Suppose we have

function f()
    produce(1)
    g()
    produce(2)
end

function g()
    produce(3)
    produce(4)
    produce(5)
end

Libtask.is_primitive(::typeof(g), args...) = false

and we decide to make a copy after hitting produce(4). If we then try to execute the instruction for g in f again, we'll end up constructing an entirely new instance of instruction g (in the current version, not yours) and so will start from produce(4) again rather than produce(5)?

EDIT: Simpler example (on #subtape, not this branch)

julia> using Libtask

julia> f() = g()
f (generic function with 1 method)

julia> function g()
           produce(1)
           produce(2)
       end
g (generic function with 1 method)

julia> Libtask.is_primitive(::typeof(g), args...) = false

julia> ttask = TapedTask(f);

julia> iterate(ttask)
(1, nothing)

julia> iterate(ttask)
(2, nothing)

julia> ttask2 = copy(ttask);

julia> iterate(ttask2)  #  BAD: starts from the beginning of `g` 
(1, nothing)

On this branch:

julia> using Libtask

julia> f() = g()
f (generic function with 1 method)

julia> function g()
           produce(1)
           produce(2)
       end
g (generic function with 1 method)

julia> Libtask.is_primitive(::typeof(g), args...) = false

julia> ttask = TapedTask(f);

julia> iterate(ttask)
(1, nothing)

julia> consume(ttask)
(2, nothing)

julia> ttask2 = copy(ttask);

julia> iterate(ttask2)  #  GOOD: does the right thing!
(nothing, nothing)

EDIT 2: The "Copying task with subtapes" test in this PR is showing the exact same thing as the above 🤦

Comment on lines 223 to 225
tf_inner = get!(tf.subtapes, instr, TapedFunction(func, inputs..., cache=true))
# continuation=false breaks "Multiple func calls subtapes" and "Copying task with subtapes"
tf_inner(inputs...; callback=callback, continuation=true)
Copy link
Member

Choose a reason for hiding this comment

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

IIUC continuation=true is now needed because we might be calling the same TapedFunction multiple times?

EDIT: In contrast to before when were just constructing a new one every time.

Copy link
Author

Choose a reason for hiding this comment

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

I think your understanding is correct.

In the tape, a function call constitutes a single instruction.
Previous to my changes, this function call was only allowed to call produce(val) once.
I think the reason was that without subtapes, we were only able to continue the execution after the call instruction.
This was safe to do if we only allow one produce and throw an error otherwise.

By flagging a function g as non-primitive is_primitive(typeof(g),...) = false, we tell Libtask that we want to be able to interrupt the execution in g (e.g. at produce(4)).

For this to work any parent tape function f (the caller) has to actually own the subtape of g.
The instruction counter of g has to be preserved, such that with continuation=true we continue exactly at the correct instruction in g. And yes, g is called multiple times with an updated counter.

Thus, when we fork/copy a task, we also have to copy all subtapes with their counters.

Also, when reusing a cached tape we have to reset all the counters of the subtapes counter = 1

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense; thank you! And again, great work:)

src/tapedfunction.jl Outdated Show resolved Hide resolved
test/tapedtask.jl Outdated Show resolved Hide resolved
test/tf.jl Outdated Show resolved Hide resolved
src/tapedfunction.jl Outdated Show resolved Hide resolved
src/tapedfunction.jl Outdated Show resolved Hide resolved
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.

3 participants