Remove TSVI or at least make TSVI opt-in

[penelopeysm]

My dislike of TSVI (ThreadSafeVarInfo) is I think well-known at this point (please see #1023 for full explanation), but just to summarise, the main issue with it is that TSVI is only required when someone uses observe ~ inside Threads.@threads, but it gets used whenever Julia is running with more than one thread.

Unfortunately, attempts to outright remove it have not really been very successful. I tried very hard in #1023 and got about 70% of the way, but there are certain technical reasons that make this a real pain to implement.

The other problem is that it is impossible at macro compile time to correctly determine whether someone is using ~ inside Threads.@threads. This is because at compile time you can only see identifiers:

julia> dump(quote
       Threads.@threads for i in 1:5
           i
       end
       end)

[...]
    2: Expr
      head: Symbol macrocall
      args: Array{Any}((3,))
        1: Expr
          head: Symbol .
          args: Array{Any}((2,))
            1: Symbol Threads
            2: QuoteNode
              value: Symbol @threads
[...]

So you only know that somebody called a macro that looks like Threads.@threads, but you don't know if it's really the same thing as Base.Threads.@threads. Besides, people might do using Threads: @threads and various other weird configurations. So, attempting to perform some automated analysis is bound to fail.

So there are two remaining options.

Remove TSVI entirely

Honestly, I am strongly in favour of just getting rid of it. I have said this several times already, there are plenty of alternatives to Threads.@threads, which do not require TSVI.

All of the other options which I listed here run perfectly fine with ordinary VarInfo and I think we should not be encouraging users to write code with Threads.@threads which is officially discouraged.

Furthermore, the ratio of code that exists and its maintenance burden, vs the number of features it actually supports, is completely disproportionate. Too much time is being spent on debugging CI and AD failures for a feature that is virtually undocumented, nobody knows that you can officially do it.

Make TSVI opt-in

The alternative is that the user must specify that they want to use TSVI, or rather, to keep it simple, the user must specify that they are using ~ within Threads.@threads.

This can be done by adding a new Boolean field to DynamicPPL.Model, which defaults to false. If the user wants to evaluate with TSVI, then they can use a function to switch this to true.

Finally, DynamicPPL.use_threadsafe_eval would just check that field instead of checking whether Threads.nthreads() > 1.

To ease the transition, we can issue a warning if the model looks like it contains Threads.@threads. This of course is not foolproof, as explained above. But it's just a warning, so we don't have to get it 100% correct: we can say "It looks like you are trying to ...".

Doing this is better than nothing. But I would still much rather prefer getting rid of TSVI.

[yebai]

I'm happy to remove TSVI entirely and find a way to warn users if they misuse threading inside models. As mentioned in #1078 (comment)

We could introduce locks at the model level. This would prevent users from using threading, except through an explicit opt-in mechanism such as a hypothetical x ~ threadsafe_product_distribution(...).

[mhauru]

I don't think this is really a problem with people using Threads.@threads. This comes up with task-based parallelism as well. For instance, consider this:

julia> module MWE

       using DynamicPPL, Distributions

       @model function f(data)
           y ~ Normal(2.0)
           ts = [
               Threads.@spawn begin
                   data[i] ~ Normal(y)
               end
               for i in eachindex(data)
           ]
           fetch.(ts)
       end

       dim = 8
       m = f(fill(0.0, dim))

       y = 1.2
       vi = SimpleVarInfo((; :y => y))
       println("Correct loglikelihood: ", logpdf(Normal(y), 0.0)*dim)
       @show getloglikelihood(last(DynamicPPL.evaluate!!(m, vi)))
       @show getloglikelihood(last(DynamicPPL.evaluate_threadunsafe!!(m, vi)))

       end
Correct loglikelihood: -13.111508265637383
getloglikelihood(last(DynamicPPL.evaluate!!(m, vi))) = -13.111508265637383
getloglikelihood(last(DynamicPPL.evaluate_threadunsafe!!(m, vi))) = -11.47256973243271

That's a reasonable looking model: You use the recommended Threads.@spawn thing to evaluate some likelihood terms in parallel. Yet it yields incorrect results without TSVI. You may say that I'm abusing Threads.@spawn by mutating a non-local variable within my task, but you can only say that if you assume that I know about __varinfo__ and how things work internally. From what's visible to the user, it's hard to explain why the above would be violating any of the recommendations of the blogpost. I don't think the post is even saying you shouldn't use @threads, but rather that you shouldn't use threadid.

I think getting rid of TSVI is better than doing nothing. The current situation is not sustainable long-term; TSVI only works most of the time because people are getting lucky. I hesitate to do it though, because I have some hope that we might make all accumulators thread-safe, which would fix this properly and allow deleting TSVI without user-breakage. If that happens soon it would be a sad to now say "you can't do ~ in parallel anymore" and then soon go "you can do it again now!" The risk of course is that we don't get to making accumulators thread-safe and the current situation persists.