Stricter types for evaluate!! methods

[mhauru]

Addresses TuringLang/Turing.jl#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%

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,

DynamicPPL.jl/src/model.jl

Line 865 in 72cfd15

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

will be invoked, and then

DynamicPPL.jl/src/model.jl

Lines 912 to 914 in 3b3840d

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

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

DynamicPPL.jl/src/model.jl

Line 865 in 72cfd15

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

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?

[devmotion]

I think it might be cleaner and more correct to just write out the varargs we want to support? For instance, args:::Union{AbstractVarInfo,AbstractSampler,AbstractContext}... will allow to specify multiple contexts or samplers etc., which is not useful at all (and will error in the next step in the evaluate!! pipeline). Similarly, instead of arbitrary numbers o f contexts as specified by ::AbstractContext... we actually only want 0 or 1 contexts.

[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.

[sunxd3]

Harmonizing with @mhauru, I agree we can improve the evalaute!! function interface. But it will have to be a future PR. For now, I think the fix is okay.

[yebai]

For consistency, maybe

Suggested change

	return evaluate!!(model, Random.default_rng(), args...)
	return AbstractPPL.evaluate!!(model, Random.default_rng(), args...)