Recent update causes method ambiguity with tilde_assume + submodel

[bgroenks96]

Hi guys,

I have some custom Model building code that builds submodels from a NamedTuple. This used to work but broke with a recent update:

    @model function priormodel(priors)
        # Case 1: Create RV following distribution `d`
        _init(name::Symbol, d::Distribution) = Turing.DynamicPPL.tilde_assume(
            __context__,
            d,
            Turing.DynamicPPL.VarName{name}(),
            (),
            __varinfo__,
        )[1]
        # Case 2: Create submodel for nested parameters (this is the expanded form of @submodel)
        _init(name::Symbol, nt::NamedTuple) = @submodel prefix=$name parammodel(priors[name])
        # Case 3: Constant parameter values
        _init(name::Symbol, val) = val
        return NamedTuple{keys(priors)}(_init(name, priors[name]) for name in keys(priors))
    end

Now produces a method ambiguity error:

ERROR: MethodError: tilde_assume(::DynamicPPL.IsParent, ::DynamicPPL.PrefixContext{:top, DynamicPPL.SamplingContext{DynamicPPL.SampleFromPrior, DynamicPPL.DefaultContext, Random._GLOBAL_RNG}}, ::Beta{Float64}, ::AbstractPPL.VarName{:winterfactor, Setfield.IdentityLens}, ::Tuple{}, ::DynamicPPL.ThreadSafeVarInfo{DynamicPPL.UntypedVarInfo{DynamicPPL.Metadata{Dict{AbstractPPL.VarName, Int64}, Vector{Distribution}, Vector{AbstractPPL.VarName}, Vector{Real}, Vector{Set{DynamicPPL.Selector}}}, Float64}, Vector{Base.RefValue{Float64}}}) is ambiguous. Candidates:
  tilde_assume(rng, context::DynamicPPL.PrefixContext, sampler, right, vn, vi) in DynamicPPL at /home/bgroenke/.julia/packages/DynamicPPL/c8MjC/src/context_implementations.jl:103
  tilde_assume(::DynamicPPL.IsParent, context::DynamicPPL.AbstractContext, args...) in DynamicPPL at /home/bgroenke/.julia/packages/DynamicPPL/c8MjC/src/context_implementations.jl:41
Possible fix, define
  tilde_assume(::DynamicPPL.IsParent, ::DynamicPPL.PrefixContext, ::Any, ::Any, ::Any, ::Any)
Stacktrace:
  [1] tilde_assume(::DynamicPPL.PrefixContext{:top, DynamicPPL.SamplingContext{DynamicPPL.SampleFromPrior, DynamicPPL.DefaultContext, Random._GLOBAL_RNG}}, ::Beta{Float64}, ::AbstractPPL.VarName{:winterfactor, Setfield.IdentityLens}, ::Tuple{}, ::DynamicPPL.ThreadSafeVarInfo{DynamicPPL.UntypedVarInfo{DynamicPPL.Metadata{Dict{AbstractPPL.VarName, Int64}, Vector{Distribution}, Vector{AbstractPPL.VarName}, Vector{Real}, Vector{Set{DynamicPPL.Selector}}}, Float64}, Vector{Base.RefValue{Float64}}})
    @ DynamicPPL ~/.julia/packages/DynamicPPL/c8MjC/src/context_implementations.jl:36

I haven't tracked down the exact issue yet. Any ideas?

Julia Version 1.6.1
Commit 6aaedecc44 (2021-04-23 05:59 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-9700K CPU @ 3.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake)
Environment:
  JULIA_EDITOR = "/home/bgroenke/.vscode-server/bin/7f6ab5485bbc008386c4386d08766667e155244e/node"
  JULIA_NUM_THREADS = 8

[bgroenks96]

Ok, found the problem... the () argument needed to be removed. What exactly changed here?

[torfjelde]

The indices argument has been removed from tilde_assume.

Also, you no longer need the $name anymore; just name should do:)

But know that you're messing with internals here, so you need to keep a track of which DynamicPPL version you are using since any breaking release can change the signature of tilde_assume, etc.

[devmotion]

But know that you're messing with internals here, so you need to keep a track of which DynamicPPL version you are using since any breaking release can change the signature of tilde_assume, etc.

Additionally, unexported functions and definitions may change even in non-breaking releases.

[torfjelde]

But know that you're messing with internals here, so you need to keep a track of which DynamicPPL version you are using since any breaking release can change the signature of tilde_assume, etc.

Additionally, unexported functions and definitions may change even in non-breaking releases.

So I didn't say this because we (for some non-apparent reason) actually export tilde_assume from DynamicPPL 😅

Btw, @bgroenks96 you're setup won't isn't guaranteed to work anymore, unfortunately. There are a couple of reasons: a) we now use DynamicPPL.tilde_assume!! as the entrance for a tilde-statement, and b) we now return a new varinfo and update the __varinfo__ in ~ statements to support immutable implementations of varinfos.

Likely a better approach to what you've written is to instead just do something like

# `NamedDist` comes from DynamicPPL.jl.
@model priormodel(name::Symbol, prior::Distribution) = x ~ NamedDist(prior, name)
# NOTE: We haven't propagated the change that `prefix=name` leads to evaluation of `name` yet, so for now we need `$` but this will soon change.
# I've also made some changes to this impl vs. yours just to get something runnable, but you get the idea.
@model priormodel(name::Symbol, nt::NamedTuple) = @submodel prefix=$name priormodel(nt)
@model priormodel(name::Symbol, val) = val

@model function priormodel(priors)
    # NOTE: The below will update `__varinfo__` for every `k`.
    vals = map(keys(priors)) do k
        @submodel priormodel(k, priors[k])
    end
    return NamedTuple{keys(priors)}(vals)
end

[bgroenks96]

That works for me! Looks much nicer too. Thanks @torfjelde !