Completely Refactor #179
Completely Refactor #179
Conversation
|
@FredericWantiez I think your problems should now be fixed. Could you let me know if it's working okay for you on the AdvancedPS side of things? |
|
Yes, that fixed the issue with the return value ! Thanks |
|
@mhauru @sunxd3 apologies in advance for the rather large PR. When reviewing, I suggest that you ignore everything in |
There was a problem hiding this comment.
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!
| @@ -11,7 +11,6 @@ jobs: | |||
| strategy: | |||
| matrix: | |||
| version: | |||
| - '1.7' | |||
| - '1.10' | |||
| - '1' | |||
| - 'nightly' | |||
There was a problem hiding this comment.
| - 'nightly' | |
| - 'pre' |
We've switched to pre over nightly in other TuringLang repos.
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | ||
| Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2" | ||
| MistyClosures = "dbe65cb8-6be2-42dd-bbc5-4196aaced4f4" | ||
| Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" |
There was a problem hiding this comment.
Is Test meant to be a strong dep of the package and not just of tests?
| Libtask.TapedTask | ||
| ``` | ||
|
|
||
| The functions discussed the above docstring (in addition to [`TapedTask`](@ref) itself) form the |
There was a problem hiding this comment.
| The functions discussed the above docstring (in addition to [`TapedTask`](@ref) itself) form the | |
| The functions discussed in the above docstring (in addition to [`TapedTask`](@ref) itself) form the |
| its `taped_globals` field. | ||
|
|
||
| The type `T` is required for optimal performance. If you know that the result of this | ||
| operation must return a specific type, specific `T`. If you do not know what type it will |
There was a problem hiding this comment.
| operation must return a specific type, specific `T`. If you do not know what type it will | |
| operation must return a specific type, specify `T`. If you do not know what type it will |
| # Extended Help | ||
|
|
||
|
|
| end | ||
| return nothing | ||
| end | ||
|
|
There was a problem hiding this comment.
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)?
| # Test case 1: stack allocated objects are deep copied. | ||
| @testset "stack allocated objects shallow copy" begin |
There was a problem hiding this comment.
The comment and the name seem conflicting.
| t[1] = 0 | ||
| while true | ||
| produce(t[1]) | ||
| t[1] |
| @test consume(a) == 4 | ||
| end | ||
| end | ||
| @testset "Issue: PR-86 (DynamicPPL.jl/pull/261)" begin |
There was a problem hiding this comment.
Am I missing something, or is this just the same test as above but with more reps?
| end | ||
|
|
||
| # Test case 2: Array objects are deeply copied. | ||
| @testset "Array objects deep copy" begin |
There was a problem hiding this comment.
Would it make sense to have a test like this but where the array returned by consume(ttask/a) is mutated by the caller?
Per #177 this is an attempt to significantly refactor the internals for robustness.
Todo:
taskfield of aTapedTaskought actually to be part of the public interface -- issue to discussion open at Accessingtaskfield of Libtask TapedTask AdvancedPS.jl#113PhiNodes are handled more thoroughly (include a worked example)optimise nested calls (currently throwing away all return type information to get prototype running)see belowLinked Issues:
taskfield of Libtask TapedTask AdvancedPS.jl#113Closes #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).