Specifying rng with DynamicPPL.evaluate!! leads to StackOverflow

[ParadaCarleton]

I assume this is to do with some kind of accidental recursion/method ambiguity; the command:

# If no sampler is specified, we sample from the prior by default
_, x1 = DynamicPPL.evaluate!!(model, rng, SimpleVarInfo(model), SamplingContext())

leads to:

StackOverflowError:

Stacktrace:
  [1] evaluate!!(::Model{typeof(demo), (:x,), (), (), Tuple{Vector{Int64}}, Tuple{}, DefaultContext}, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Vararg{Any})
    @ DynamicPPL ~/.julia/dev/DynamicPPL.jl/src/model.jl:420
  [2] evaluate!!(::Model{typeof(demo), (:x,), (), (), Tuple{Vector{Int64}}, Tuple{}, DefaultContext}, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Random._GLOBAL_RNG, ::Vararg{Any}) (repeats 10911 times)
    @ DynamicPPL ~/.julia/dev/DynamicPPL.jl/src/model.jl:421
  [3] top-level scope
    @ ~/.julia/dev/DynamicPPL.jl/docs/src/tutorials/met_hast_dppl.ipynb:2
  [4] eval
    @ ./boot.jl:373 [inlined]
  [5] include_string(mapexpr::typeof(REPL.softscope), mod::Module, code::String, filename::String)
    @ Base ./loading.jl:1196
  [6] #invokelatest#2
    @ ./essentials.jl:716 [inlined]
  [7] invokelatest
    @ ./essentials.jl:714 [inlined]
  [8] (::VSCodeServer.var"#150#151"{VSCodeServer.NotebookRunCellArguments, String})()
    @ VSCodeServer ~/.vscode/extensions/julialang.language-julia-1.5.11/scripts/packages/VSCodeServer/src/serve_notebook.jl:18
  [9] withpath(f::VSCodeServer.var"#150#151"{VSCodeServer.NotebookRunCellArguments, String}, path::String)
    @ VSCodeServer ~/.vscode/extensions/julialang.language-julia-1.5.11/scripts/packages/VSCodeServer/src/repl.jl:185
 [10] notebook_runcell_request(conn::VSCodeServer.JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint}, params::VSCodeServer.NotebookRunCellArguments)
    @ VSCodeServer ~/.vscode/extensions/julialang.language-julia-1.5.11/scripts/packages/VSCodeServer/src/serve_notebook.jl:14
 [11] dispatch_msg(x::VSCodeServer.JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint}, dispatcher::VSCodeServer.JSONRPC.MsgDispatcher, msg::Dict{String, Any})
    @ VSCodeServer.JSONRPC ~/.vscode/extensions/julialang.language-julia-1.5.11/scripts/packages/JSONRPC/src/typed.jl:67
 [12] serve_notebook(pipename::String; crashreporting_pipename::String)
    @ VSCodeServer ~/.vscode/extensions/julialang.language-julia-1.5.11/scripts/packages/VSCodeServer/src/serve_notebook.jl:94
 [13] top-level scope
    @ ~/.vscode/extensions/julialang.language-julia-1.5.11/scripts/notebook/notebook.jl:12
 [14] include(mod::Module, _path::String)
    @ Base ./Base.jl:418
 [15] exec_options(opts::Base.JLOptions)
    @ Base ./client.jl:292
 [16] _start()
    @ Base ./client.jl:495

[ParadaCarleton]

(Removing the RNG leads it to run as expected)

[devmotion]

xref: #360 (comment)

[devmotion]

Speecifying both a sampling context and an RNG is redundant, hence IMO this should not be supported. But as mentioned in the comment linked above, we should clean the dispatches and function signatures such that it throws a MethodError instead of a StackOverflowError.

[ParadaCarleton]

Speecifying both a sampling context and an RNG is redundant, hence IMO this should not be supported. But as mentioned in the comment linked above, we should clean the dispatches and function signatures such that it throws a MethodError instead of a StackOverflowError.

Makes sense, but then the docstring should be fixed, as currently it says:

  evaluate!!(model::Model[, rng, varinfo, sampler, context])

Which implies it should be possible to pass both an rng and a context.

[ParadaCarleton]

_, x4 = DynamicPPL.evaluate!!(model, x1, SamplingContext(rng))

Hmm, this still throws an error:

MethodError: no method matching DynamicPPL.SamplingContext(::Random.MersenneTwister)

Closest candidates are:

DynamicPPL.SamplingContext(::R, !Matched::S, !Matched::C) where {S<:AbstractMCMC.AbstractSampler, C<:DynamicPPL.AbstractContext, R} at ~/.julia/packages/DynamicPPL/c8MjC/src/contexts.jl:131

DynamicPPL.SamplingContext(::Any, !Matched::Any) at ~/.julia/packages/DynamicPPL/c8MjC/src/contexts.jl:135

DynamicPPL.SamplingContext() at ~/.julia/packages/DynamicPPL/c8MjC/src/contexts.jl:138

...

top-level scope@[Local: 1](http://localhost:1234/edit?id=159c8e8c-8cf4-11ec-0a77-4b4454424b8c#)[inlined]

[ParadaCarleton]

Ahh, ok, looks like it requires either all the arguments to be passed or none of them. So this works:

SamplingContext(rng, SampleFromPrior(), DefaultContext())

But dropping SampleFromPrior() or DefaultContext() throws an error. Should I make a PR setting these as defaults?

[devmotion]

Ahh, ok, looks like it requires either all the arguments to be passed or none of them.

No, there are a bunch of different constructors:

DynamicPPL.jl/src/contexts.jl

Lines 135 to 138 in aeb5e03

	SamplingContext(sampler, context) = SamplingContext(Random.GLOBAL_RNG, sampler, context)
	SamplingContext(context::AbstractContext) = SamplingContext(SampleFromPrior(), context)
	SamplingContext(sampler::AbstractSampler) = SamplingContext(sampler, DefaultContext())
	SamplingContext() = SamplingContext(SampleFromPrior())

You can pass only a sampler, only a child context, a sampler and a child context, or an RNG, a sampler, and a child context. But it seems we could add some additional constructors and change it to e.g.

function SamplingContext(
    rng::AbstractRNG=Random.GLOBAL_RNG, sampler::AbstractSampler=SampleFromPrior()
)
    return SamplingContext(rng, sampler, DefaultContext())
end
function SamplingContext(rng::AbstractRNG, context::AbstractContext)
    return SamplingContext(rng, SampleFromPrior(), context)
end
function SamplingContext(sampler::AbstractSampler, context::AbstractContext=DefaultContext())
    return SamplingContext(Random.GLOBAL_RNG, sampler, context)
end
function SamplingContext(context::AbstractContext)
    return SamplingContext(Random.GLOBAL_RNG, SampleFromPrior(), context)
end

[ParadaCarleton]

That looks good to me.

[yebai]

@mhauru is this fixed now recently?

[mhauru]

Yes, fixed by #629