Stricter types for evaluate!! methods

[mhauru]

Addresses https://github.com/TuringLang/Turing.jl/issues/2182

The earlier

function AbstractPPL.evaluate!!(model::Model, args...)
    return evaluate!!(model, Random.default_rng(), args...)
end

could lead to infinite recursion. Restricting the types of the varargs solves that.

I would have wanted to rework the method definitions of evaluate!! more generally because I find it hard to understand what all the valid ways of calling it are. I thought about it for a bit and couldn't come up with a good solution though, so maybe later.

[coveralls]

Pull Request Test Coverage Report for Build 9909759705

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 24 unchanged lines in 4 files lost coverage.
• Overall coverage remained the same at 81.419%

---

Files with Coverage Reduction	New Missed Lines	%
src/model.jl	1	87.0%
src/simple_varinfo.jl	5	82.18%
src/varinfo.jl	6	84.85%
src/threadsafe.jl	12	46.73%
<!--	Total:	24		-->

Totals
Change from base Build 9793413975:	0.0%
Covered Lines:	2800
Relevant Lines:	3439

---

💛 - Coveralls

[sunxd3]

Looks good! Feels like there might be some hidden implications, so let me take a look now.

[sunxd3]

My reading of the issue is this: sometimes user confuse the "model creating function" with the functor DynamicPPL.Model. They are both "callable"s, so somebody may want to create a Model, but called an already created Model on the input data.

In this case, https://github.com/TuringLang/DynamicPPL.jl/blob/72cfd15ef8731951c2e051fa06c78319eb4ad8ac/src/model.jl#L865 will be invoked, and then https://github.com/TuringLang/DynamicPPL.jl/blob/3b3840d57d67ea2d9fc56b5bc927d1e0be89a828/src/model.jl#L912-L914 At this time, infinite recurse will occur, because no other evaluate!! will be type-matched.

Current proposed resolution works, but I wonder maybe we can just combine the two functions above, i.e., modifying https://github.com/TuringLang/DynamicPPL.jl/blob/72cfd15ef8731951c2e051fa06c78319eb4ad8ac/src/model.jl#L865 to

(model::Model)(args...) = first(evaluate!!(model, Random.default_rng(), args...))

Would this be cleaner? I am not sure.

Additionally, the thrown error will be something like

ERROR: MethodError: no method matching evaluate!!(::Model{…}, ::Vector{…}, ::Vector{…})
The function `evaluate!!` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  evaluate!!(::Model, ::AbstractVarInfo, ::AbstractContext)
   @ DynamicPPL ~/DynamicPPL.jl/src/model.jl:888
  evaluate!!(::Model, ::Random.AbstractRNG)
   @ DynamicPPL ~/DynamicPPL.jl/src/model.jl:898
  evaluate!!(::Model, ::Random.AbstractRNG, ::AbstractVarInfo)
   @ DynamicPPL ~/DynamicPPL.jl/src/model.jl:898
  ...

which, arguably is still not great. Because it exposes the un-exported evaluate!! function, and not sure user will know what to do with the message. But on this, I don't have a good solution.

[mhauru]

With (model::Model)(args...) = first(evaluate!!(model, Random.default_rng(), args...)) if the user provides an rng as part of args evaluate!! would then be called with two rngs in its arguments, which would be trouble.

Is your main wish here to have the error message be clearer, or for the code to be cleaner?

Even if we change the relation of (model::Model)(args...) and evaluate!!, I would still propose adding the varargs type bounds for evaluate!!, because someone might end up calling evaluate!! directly or through other routes, and having any function have the possiblity of going into infinite recursion I think is worth fixing.

[sunxd3]

Is your main wish here to have the error message be clearer, or for the code to be cleaner? I thought it makes the code cleaner.

I don't have objections to type-restrict evaluate!!. Thoughts @yebai @torfjelde?

[mhauru]

I agree @devmotion, but I wonder if we embark on that we should actually rework the method definitions generally to make the whole definition of evaluate!! easier to read. To be clear, I'm not proposing breaking changes, but rather readability improvements that also catch corner case bugs like the one this one is fixing.