Completely Refactor

[willtebbutt]

Per #177 this is an attempt to significantly refactor the internals for robustness.

Todo:

• understand what the desired semantics are regarding copying, per the discussion in Questions About Semantics #178
• determine whether the task field of a TapedTask ought actually to be part of the public interface -- issue to discussion open at Accessing task field of Libtask TapedTask AdvancedPS.jl#113
• implement an opt-in mechanism for nested calls containing produce statements
• tidy up main body of code
• document how PhiNodes are handled more thoroughly (include a worked example)
• document the intent of the code + the high-level structure. In particular, re-work the README to explain what the public interface is, and what it does.
• get all CI passing
• check that we can make AdvancedPS and Turing work using this PR
• kwarg support
• improve nesting mechanism to ensure that we always catch everything
• optimise nested calls (currently throwing away all return type information to get prototype running) see below
• benchmark to ensure no regressions
• add BBCode section to Mooncake docs so that this implementation is intelligible to everyone
• add more high-level documentation

Linked Issues:

• Significantly improve the Libtask library using ideas from Mooncake / ReverseDiff #177
• Questions About Semantics #178
• Accessing task field of Libtask TapedTask AdvancedPS.jl#113

Closes #165
Closes #167
Closes #171
Closes #176

edit: performance optimisation. Everything (as far as I know) is now type-stable where it ought to be, except in the context of nested calls. These are somewhat harder to achieve optimal performance with (you need the IR of the thing that you're going to recurse into in order to figure out the return type of a call-which-might-produce -- since nasty things like recursion exist, it's not completely trivial to do this correctly).

[FredericWantiez]

Yes, that fixed the issue with the return value ! Thanks

[willtebbutt]

@mhauru @sunxd3 apologies in advance for the rather large PR.

When reviewing, I suggest that you ignore everything in src/bbcode.jl and src/utils.jl -- they are copies of utility functionality from Mooncake. Additionally, if anything is unclear, please let me know rather than trying to spend too long to understand it if you feel that it is under-documented -- I'm keen to add additional docs etc, so it's really rather helpful to know where things aren't straightforward to understand.

[mhauru]

I didn't read the last 700 lines of src/copyable_task.jl, nor did read through the deleted tests to see that the new ones cover the same cases. I may come to those later, but figured leaving some comments now would be helpful.

I only have some localised comments and questions, and a couple of design questions. Mostly looks great, thanks so much for doing this!

[mhauru]

The test cases made me wonder, what happens if I do

f = Libtask.produce
f(0)

or something like

using Libtask: produce as prod

prod(0)

?

[willtebbutt]

Good questions. I'll add tests for this (I think it should be fine).

[mhauru]

The comment and the name seem conflicting.

[willtebbutt]

Interesting. This test is just copied over from the old test suite, so I'm not sure what the intention was.

[yebai]

cc @KDr2

[KDr2]

Here, the accurate description may be "copied by value". Back to the C-implemented version, the behavior is:

• For primitive number types, the value is copied (not deep and not shallow)
• For reference to an object, the reference is copied (not deep and not shallow, just copy the reference by the pointer value)
• Mutable objects allocated to the stack are shallow copied.

But now in the Pure-Julia version, I think only the first item holds. Maybe we should change these lines to copied by value?

[mhauru]

What does this line do?

[willtebbutt]

I don't believe that it does anything -- I'll remove. This is another test that I've copied straight over from the old test suite.

[KDr2]

It's about an issue of the C-implemented version (maybe data initialization and stack size related). We can safely remove them(including the next similar test case) now.

[mhauru]

Am I missing something, or is this just the same test as above but with more reps?

[willtebbutt]

Possibly -- I've just left this alone because it appears to be in relationship to a specific issue.

[mhauru]

Would it make sense to have a test like this but where the array returned by consume(ttask/a) is mutated by the caller?

[willtebbutt]

Could you elaborate a little bit?

[mhauru]

I'm wondering about something like

            function f()
                t = [0 1 2]
                while true
                    produce(t)
                    t[1] = 1 + t[1]
                end
            end

            ttask = TapedTask(nothing, f)
            t = consume(ttask)
            @test t[1] == 0
            consume(ttask)
            @test t[1] == 1
            t[1] = -2
            consume(ttask)
            @test t[1] == -1

Or maybe that's not how it works if a copy is made, but then there could be a test to ensure that indeed a copy is made and the above doesn't work.

[sunxd3]

Suggested change

	# where `deref_phi` retrives the value in position `ref_ind_for_ID(%n)` if
	# where `deref_phi` retrieves the value in position `ref_ind_for_ID(%n)` if

[sunxd3]

This is marvelous effort.

I read through src/copyable_task.jl and I think I understand the logic, but I haven't gone through the tests.

I don't have much comment on coding style and fine-grain logics. But I want to mention some points that would be great if they can be included in the documentation:

1. I think get_taped_globals using task_local_storage is a great idea, but maybe explain a bit how and why to use task_local_storage? (I recall this is to support some necessary state for SMC like RNG?)
2. It tooks me a bit of effort to understand how refs is used to enable isolation between execution of copied TapedTask, some high level description would help a lot (maybe this already exists and I just missed it)

[sunxd3]

I assume the alternative values arg can take is something like Const?

[sunxd3]

I seems to me that the block following this assumes that the split is successful and might_produce is placed at the end of the basic block. Is there a simple way to assert this assumption?

[sunxd3]

Would this be dangerous in multi-thread settings?