[email protected] Compatibility Fix

[sunxd3]

The test is failing at two main points:

1. DynamicPPL.jl/test/varinfo.jl

   Line 58 in e6dd4ef

   @test inspace(@varname(M[:, 1:4]), space)

   This test should arguably be marked as broken, unless there is a specific reason for its inclusion?

2. DynamicPPL.jl/test/varinfo.jl

   Line 347 in e6dd4ef

   any(DynamicPPL.subsumes(vn_left, vn) for vn_left in keys(vals))

   with model

   DynamicPPL.jl/src/test_utils.jl

   Lines 549 to 562 in e6dd4ef

   	@model function demo_assume_matrix_dot_observe_matrix(
   	x=transpose([1.5 2.0;]), ::Type{TV}=Array{Float64}
   	) where {TV}
   	n = length(x)
   	d = n ÷ 2
   	s ~ reshape(product_distribution(fill(InverseGamma(2, 3), n)), d, 2)
   	s_vec = vec(s)
   	m ~ MvNormal(zeros(n), Diagonal(s_vec))
   	# Dotted observe for `Matrix`.
   	x .~ MvNormal(m, Diagonal(s_vec))
   	return (; s=s, m=m, x=x, logp=getlogp(__varinfo__))
   	end

   Two potential solutions come to mind. We could either use == instead of subsume (or in) in the test, or we could modify subsume to allow two equal varnames to subsume each other.

The issue of concretization runs a bit deeper, and the following concerns warrant discussion:

1. At https://github.com/TuringLang/AbstractPPL.jl/blob/0f289206b22da5feee03218c157afb28706ed8ec/src/varname.jl#L609 the decision to concretize is based on Setfield.need_dynamic_lens, but expressions with Colon return false. Should this be changed?

2. There should be no issues using offset arrays and infinite dimensional models. However, the concern lies in user-friendliness, which is linked to the next point.

3. The automatic concretization could potentially confuse intermediate users and lead to silent failures in code pieces we gave to users. This is especially problematic when the functions use varname as keys and key comparison becomes more complex because of the concretize v.s. unconretized dichotomy, use of offset arrays can potentially make this worse. Additionally, (c.f. [Merged by Bors] - Concretize IndexLens using to_indices AbstractPPL.jl#43 (comment)), say one doesn't have x in the environment, but does @varname(x[end]), it will error, and this could be potentially very confusing and explain why by default the concretize was set to false.

Fix #440; Fix #470

[sunxd3]

In DPPL, do we need concrete varname to guarantee correctness or concreteness is just a nice feature?

[yebai]

@torfjelde @devmotion, what are your thoughts here?

[torfjelde]

Will have a look on Friday.

[sunxd3]

Okay, I have narrowed down the fail cases in SimpleVarInfo tests to

DynamicPPL.jl/src/utils.jl

Lines 338 to 341 in e6dd4ef

	function set!!(obj, vn::VarName{sym}, value) where {sym}
	lens = BangBang.prefermutation(Setfield.PropertyLens{sym}() ∘ AbstractPPL.getlens(vn))
	return Setfield.set(obj, lens, value)
	end

which is called from

DynamicPPL.jl/src/simple_varinfo.jl

Lines 337 to 340 in e6dd4ef

	function BangBang.setindex!!(vi::SimpleVarInfo, val, vn::VarName)
	# For `NamedTuple` we treat the symbol in `vn` as the _property_ to set.
	return Setfield.@set vi.values = set!!(vi.values, vn, val)
	end

BangBang.set!! silently modify the eltype of some array from Float64 to Any, MWE

julia> model = DynamicPPL.TestUtils.demo_dot_assume_matrix_dot_observe_matrix();

julia> values_constrained = rand(NamedTuple, model)
(s = [1.1595043641715013 2.778153502600599], m = [0.32505356054876894, 0.3961233885884393])

julia> vi = SimpleVarInfo(values_constrained);

julia> vn = DynamicPPL.TestUtils.varnames(model)[1]
s[:, 1] # this is conretized

julia> DynamicPPL.setindex!!(vi, [1.0], vn)
SimpleVarInfo((s = Any[1.0 1.3154718042147129], m = [2.129624970695997, 0.849323875002674]), 0.0)

julia> AbstractPPL.set(vi.values, vn, [1.0])
(s = [1.0 1.3154718042147129], m = [2.129624970695997, 0.849323875002674])

[sunxd3]

@torfjelde any chance you seen this before?

[sunxd3]

It might be desirable to set concretize to false by default at https://github.com/TuringLang/AbstractPPL.jl/blob/0f289206b22da5feee03218c157afb28706ed8ec/src/varname.jl#L609
so that it don't error if someone does @varname(s[end]) when there is no s in the environment. But this would requires we modify getindexs to allow indexing with un-concretized varnames.

[github-actions]

Pull Request Test Coverage Report for Build 5621778213

• 12 of 12 (100.0%) changed or added relevant lines in 3 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.02%) to 76.325%

---

Files with Coverage Reduction	New Missed Lines	%
src/utils.jl	2	76.96%

Totals
Change from base Build 5621457734:	0.02%
Covered Lines:	2060
Relevant Lines:	2699

---

💛 - Coveralls

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (0d5c463) 76.30% compared to head (dddda4d) 76.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   76.30%   76.32%   +0.02%     
==========================================
  Files          22       22              
  Lines        2688     2699      +11     
==========================================
+ Hits         2051     2060       +9     
- Misses        637      639       +2     

Impacted Files	Coverage Δ
src/compiler.jl	93.20% <100.00%> (+0.27%)	⬆️
src/test_utils.jl	83.24% <100.00%> (+0.04%)	⬆️
src/utils.jl	76.95% <100.00%> (-0.72%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[sunxd3]

Okay, I modify the set!! function like #494 (comment). The tests are passing. But this is not a good fix as maybe we want to modify the underlying object with set!!?

[yebai]

@devmotion @torfjelde what are your thoughts on @sunxd3's proposed fixes?

[torfjelde]

Having a look now 👍

[torfjelde]

Okay, I modify the set!! function like #494 (comment). The tests are passing. But this is not a good fix as maybe we want to modify the underlying object with set!!?

Yeah, I don't think this is the way to go 😕

What's the actual issue?

So a few things:

1. If we hit the mutating branch in set!!, then it will not change the type (since it will then try to mutate the buffer).
2. But set!! can also hit set if it cannot find a mutating version.

This means that we're hitting scenario (2).

If you use something like Cthulhu.jl to step through the call in your example where set!! results in an incorrect type, following the resulting set calls, you eventually see the following:

_setindex(xs::AbstractArray, v, I...) @ BangBang.NoBang ~/.julia/packages/BangBang/FUkah/src/NoBang/base.jl:129
129 function _setindex(xs::Matrix{Float64}::AbstractArray, v::Vector{Float64}, I::Tuple{AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}...)::Matrix{Any}
130     T::Core.Const(Any) = promote_type(eltype(xs::Matrix{Float64})::Type{Float64}, typeof(v::Vector{Float64})::Type{Vector{Float64}})::Type{Any}
131     ys::Matrix{Any} = similar(xs::Matrix{Float64}, T::Type{Any})::Matrix{Any}
132     if (eltype(xs::Matrix{Float64})::Type{Float64} !== Union::Type{Union}{}::Type{Union{}})::Bool
133         copy!(ys::Matrix{Any}, xs::Matrix{Float64})
134     end
135     ys::Matrix{Any}[I::Tuple{AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}...] = v
136     return ys::Matrix{Any}
137 end
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
   eltype(xs::Matrix{Float64})
   promote_type(eltype(xs::Matrix{Float64})::Type{Float64}, typeof(v::Vector{Float64})::Type{Vector{Float64}})
   similar(xs::Matrix{Float64}, T::Type{Any})
   eltype(xs::Matrix{Float64})
    eltype(xs::Matrix{Float64})::Type{Float64} !== Union::Type{Union}{}::Type{Union{}}
   %9 = copy!(::Matrix{Any},::Matrix{Float64})::Any
   %12 = setindex!(::Matrix{Any},::Vector{Float64},::AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}},::Int64)::Any
 • ↩

This is very verbose, but notice in particular the following part

   eltype(xs::Matrix{Float64})
   promote_type(eltype(xs::Matrix{Float64})::Type{Float64}, typeof(v::Vector{Float64})::Type{Vector{Float64}})
   similar(xs::Matrix{Float64}, T::Type{Any})

Here it becomes clear why we're ending up with Any: promote_type is called on eltype(xs::Matrix{Float64})::Type{Float64} and typeof(v::Vector{Float64})::Type{Vector{Float64}}, thus trying to promote a Vector{Float64} into a Float64, which returns Type{Any}.

So that answers the questions of where the Any comes from. It does not however answer the question

Why are we even hitting this non-mutating version in the first place?

When you step through using Cthulhu.jl, before you reach the _setindex we saw above, you'll also hit this:

may(mutate, args...) @ BangBang ~/.julia/packages/BangBang/FUkah/src/core.jl:7
┌ Warning: Some line information is missing, type-assignment may be incomplete
└ @ Cthulhu ~/.julia/packages/Cthulhu/vJrGb/src/codeview.jl:142
 7  may(mutate::typeof(BangBang._setindex!), args::Tuple{Matrix{Float64}, Vector{Float64}, AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}...)::Matrix{Any} =
 8     if possible(mutate::typeof(BangBang._setindex!), args::Tuple{Matrix{Float64}, Vector{Float64}, AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}...)
 9         mutate(args...)
10     else
11         pure(mutate::typeof(BangBang._setindex!))::typeof(BangBang.NoBang._setindex)(args::Tuple{Matrix{Float64}, Vector{Float64}, AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}...)::Matrix{Any}
12     end
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
   %4 = possible(::#_setindex!,::Array{…},::Array{…},::ConcretizedSlice{…},::Int64)::Core.Const(false)
   pure(mutate::typeof(BangBang._setindex!))
 • pure(mutate::typeof(BangBang._setindex!))::typeof(BangBang.NoBang._setindex)(args::Tuple{Matrix{Float64}, Vector{Float64}, A…
   ↩

where, in particular,

   %4 = possible(::#_setindex!,::Array{…},::Array{…},::ConcretizedSlice{…},::Int64)::Core.Const(false)

where we see this BangBang.possible function returns false, i.e. it considers it not possible to mutate.

Looking at the source in the above, i.e.

 8     if possible(mutate::typeof(BangBang._setindex!), args::Tuple{Matrix{Float64}, Vector{Float64}, AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}...)
 9         mutate(args...)

we see that BangBang.possible is given args::Tuple{Matrix{Float64}, Vector{Float64}, AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}, and given what we saw before with Any, I'm at this poin suspecting that a similar thing is causing possible to return false: Vector{Float64} is not considered a valid element of Matrix{Float64}, even with a ConcretizedSlice using Base.OneTo.

But let's check. Stepping into possible

possible(::typeof(BangBang._setindex!), ::C, ::T, ...) where {C<:AbstractArray, T} @ BangBang ~/.julia/packages/BangBang/FUkah/src/base.jl:484
484 (possible(::(typeof(_setindex!))::typeof(BangBang._setindex!), ::(C)::Matrix{Float64}, ::(T)::Vector{Float64}, ::(Vararg)::Tuple{AbstractPPL.ConcretizedSlice{Int64, Base.OneTo{Int64}}, Int64}) where {C <: AbstractArray, T})::Core.Const(false) =
485     implements(setindex!, C::Type{Matrix{Float64}}) && promote_type(eltype(C::Type{Matrix{Float64}}), T::Type{Vector{Float64}})::Type{Any} <: eltype(C::Type{Matrix{Float64}})::Bool
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
 • %1 = implements(::typeof(setindex!),::Type{Matrix{Float64}})::Core.Const(true)
   %3 = eltype(::Type{Matrix{Float64}})::Core.Const(Float64)
   promote_type(eltype(C::Type{Matrix{Float64}}), T::Type{Vector{Float64}})
   %5 = eltype(::Type{Matrix{Float64}})::Core.Const(Float64)
   ↩

where in particular, we see the line

485     implements(setindex!, C::Type{Matrix{Float64}}) && promote_type(eltype(C::Type{Matrix{Float64}}), T::Type{Vector{Float64}})::Type{Any} <: eltype(C::Type{Matrix{Float64}})::Bool

where indeed Any pops up in a similar pomote_type as we saw earlier.

The solution

To solve this, it seems that we need to tell BangBang that it is indeed okay to call setindex! on AbstractArray{N,T} with element being of type AbstractArray{M,T} if given a ConcretizedSlice with M entries being slices.

More concretely, we need to implement BangBang.possible for this combination of inputs. As seen above, the current implementation we're hitting is

possible(::typeof(_setindex!), ::C, ::T, ::Vararg) where {C <: AbstractArray, T} =
    implements(setindex!, C) && promote_type(eltype(C), T) <: eltype(C)

and so maybe doing something like

_dimension(index) = 0
_dimension(::AbstractPPL.ConcretizedSlice) = 1
_dimension(indices::Tuple) = sum(map(_dimension, indices))


function BangBang.possible(
    ::typeof(BangBang._setindex!),
    ::C,
    ::T,
    indices::Vararg
) where {N, M, C <: AbstractArray{<:Any,N}, T <: AbstractArray{<:Any,M}}
    return BangBang.implements(setindex!, C) &&
        promote_type(eltype(C), eltype(T)) <: eltype(C) &&
        _dimension(indices) == M
end

With this method impleemnted, we have

julia> model = DynamicPPL.TestUtils.demo_dot_assume_matrix_dot_observe_matrix();

julia> values_constrained = rand(NamedTuple, model)
(s = [4.895152314885813 2.111286552161793], m = [0.8664750151981476, 1.6153565968318373])

julia> vi = SimpleVarInfo(values_constrained);

julia> vn = DynamicPPL.TestUtils.varnames(model)[1]
s[:,1]

julia> DynamicPPL.setindex!!(vi, [1.0], vn)
SimpleVarInfo((s = [1.0 2.111286552161793], m = [0.8664750151981476, 1.6153565968318373]), 0.0)

as wanted!

But this should really be in BangBang.jl IMO. I'll make a PR.

[torfjelde]

Related: JuliaFolds/BangBang.jl#238

Once that is added, we really only need to overload _index_dimension for ConcretizedSlice in AbstractPPL, and we should be gucci.

[sunxd3]

Nice 🕵️‍♂️ work, I'll monitor the PR and follow up later.

[yebai]

thanks, @sunxd3 and @torfjelde for the investigations! Let's aim to fix this compact issue quickly since it has been causing various issues for a while.

[sunxd3]

While recreating the debugging process of @torfjelde, I realized I didn't answer why with unconcretized varname, things worked fine. After a little bit digging, I found a function Tor wrote to make it work.
Maybe the BangBang PR is a better solution in the long run, but in the short term, a similar function to above should do for concretized varname.

[yebai]

Good work @sunxd3 — I left one clarification comment below.

[yebai]

@torfjelde Can you take a quick look before I merge this PR? Let's aim to merge it in the next day or two.