-
Notifications
You must be signed in to change notification settings - Fork 230
Switch to new Libtask and support Julia 1.7 #1744
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
Conversation
rikhuijzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
Also, the julia compat entry can be reverted (#1740) now that Julia 1.7 will be supported again.
|
CI setup has to be reverted as well, currently only 1.3 and 1.6 are tested. |
|
https://github.com/TuringLang/Turing.jl/blob/master/src/inference/AdvancedSMC.jl#L354 The Any good idea to fix this? @yebai |
|
We might have to trace into |
|
@KDr2 can you share an example of the complete call stack that leads to |
|
I have made
2: about random data generating (I'm not sure)
|
@KDr2 I checked carefully, and it appears that we only called EDIT: can you check whether another top-level instruction called |
|
A useful debugging trick is to print the call stack while inside |
|
BT1: BT2 It seems they are the same path to produce, but when the second reaches UPDATE: |
That explains the current behaviour! |
|
And now, the errors become back to we can't get the IR code of some functions, below is a non-exhaustive list:
|
|
The stack overflow still exists, we didn't see it the last time we ran tests last night, because the Julia process was stuck on my machine :( |
|
@KDr2 The stackflow error is caused by these lines (m::DynamicPPL.Model)(x)=m(Random.GLOBAL_RNG, x)
(m::DynamicPPL.Model)(x, y)=m(Random.GLOBAL_RNG, x, y)
(m::DynamicPPL.Model)(x, y, z)=m(Random.GLOBAL_RNG, x, y, z)I guess these new function definitions do not specify argument types, so the downstream calls to
I fixed these lines by adding argument type information in 0cfb35e and d4eeeb9 EDIT: it seems CI is still using the wrong branch of |
|
@yebai changed the calls to Model to: (m::DynamicPPL.Model)(x::DynamicPPL.AbstractVarInfo)=first(DynamicPPL.evaluate!!(m, Random.GLOBAL_RNG, x))but that runs into convert errors. |
|
I can get past it if I intercept the instruction we use to intercept return values: function (instr::Instruction{F})() where F
output = instr.fun(map(val, instr.input)...)
if instr.fun == identity
instr.output = box(output)
else
instr.output.val = output
end
endNow the model would run but fails on the resampling step, looks like the copied particles do not resume properly, returning |
|
@FredericWantiez thanks, can you push your code please? @KDr2 the remaining issue is likely associated with the new Libtask, can you take a look? |
|
@KDr2 we might need to add the function ‘first’ to traceable functions. Might be useful to carefully check whether we correctly trace every function again now that the code can run. |
| (m::DynamicPPL.Model)(x::DynamicPPL.AbstractVarInfo)=first(DynamicPPL.evaluate!!(m, Random.GLOBAL_RNG, x)) | ||
| (m::DynamicPPL.Model)(x::DynamicPPL.AbstractVarInfo, y::DynamicPPL.AbstractSampler)=first(DynamicPPL.evaluate!!(m, Random.GLOBAL_RNG, x, y)) | ||
| (m::DynamicPPL.Model)(x::DynamicPPL.AbstractVarInfo, y::DynamicPPL.AbstractSampler, z::DynamicPPL.AbstractContext)=first(DynamicPPL.evaluate!!(m, Random.GLOBAL_RNG, x, y, z)) | ||
|
|
||
| # trace down into | ||
| Libtask.trace_into(::DynamicPPL.Model) = true | ||
| Libtask.trace_into(::typeof(DynamicPPL.evaluate_threadsafe!!)) = true | ||
| Libtask.trace_into(::typeof(DynamicPPL.evaluate_threadunsafe!!)) = true | ||
| Libtask.trace_into(::typeof(DynamicPPL._evaluate!!)) = true | ||
| Libtask.trace_into(::typeof(DynamicPPL.tilde_observe)) = true | ||
| Libtask.trace_into(::typeof(DynamicPPL.tilde_observe!!)) = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These definitions are quite severe type piracies, in particular the ones for (m::Model)(...). Is the plan to move these definitions to DynamicPPL once tests pass and before this PR is merged? The definitions of trace_into would require that DynamicPPL depends on Libtask though, is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a cleaner way to implement the new Libtask-Turing integration. However, for now, we are trying to make things work together. This code will be replaced later.
|
@KDr2 I just re-run the tests. Code is now running but there is one issue
This normally means different Note: it's fine to ignore numerical errors for now because without adding support for stochastic control flows in Libtask, some models like |
|
We might need to update the following function in order to use the new Turing.jl/src/core/container.jl Line 18 in ac99fb0
|
3a86bb8 to
24cdbde
Compare
|
Replaced by #1757 |
No description provided.