Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transition to Accessors.jl #585

Merged
merged 29 commits into from
Apr 18, 2024
Merged

Transition to Accessors.jl #585

merged 29 commits into from
Apr 18, 2024

Conversation

sunxd3
Copy link
Member

@sunxd3 sunxd3 commented Apr 5, 2024

Fix #584 Fix #583 Fix #572

src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
sunxd3 and others added 4 commits April 5, 2024 12:43
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@sunxd3
Copy link
Member Author

sunxd3 commented Apr 7, 2024

current error caused by TuringLang/AbstractPPL.jl#93

test/runtests.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
sunxd3 and others added 4 commits April 9, 2024 09:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
test/runtests.jl Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 12, 2024

Pull Request Test Coverage Report for Build 8740059589

Details

  • 78 of 95 (82.11%) changed or added relevant lines in 7 files are covered.
  • 103 unchanged lines in 10 files lost coverage.
  • Overall coverage decreased (-3.1%) to 81.855%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/varinfo.jl 5 6 83.33%
src/model_utils.jl 2 4 50.0%
src/simple_varinfo.jl 16 18 88.89%
src/utils.jl 42 45 93.33%
src/threadsafe.jl 9 18 50.0%
Files with Coverage Reduction New Missed Lines %
src/sampler.jl 1 94.12%
ext/DynamicPPLForwardDiffExt.jl 1 77.78%
src/contexts.jl 2 77.27%
src/logdensityfunction.jl 2 55.56%
src/prob_macro.jl 4 84.67%
src/abstract_varinfo.jl 5 82.68%
src/context_implementations.jl 8 61.96%
src/model_utils.jl 10 19.64%
src/loglikelihoods.jl 16 54.84%
src/varinfo.jl 54 86.72%
Totals Coverage Status
Change from base Build 8740001884: -3.1%
Covered Lines: 2621
Relevant Lines: 3202

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8658853351

Details

  • 74 of 95 (77.89%) changed or added relevant lines in 7 files are covered.
  • 104 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-4.0%) to 80.823%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/varinfo.jl 5 6 83.33%
src/model_utils.jl 2 4 50.0%
src/simple_varinfo.jl 16 18 88.89%
src/utils.jl 42 45 93.33%
src/threadsafe.jl 5 18 27.78%
Files with Coverage Reduction New Missed Lines %
src/compiler.jl 1 93.69%
src/sampler.jl 1 94.12%
ext/DynamicPPLForwardDiffExt.jl 1 77.78%
src/contexts.jl 2 77.27%
src/logdensityfunction.jl 2 55.56%
src/prob_macro.jl 4 84.67%
src/abstract_varinfo.jl 5 81.97%
src/context_implementations.jl 8 61.96%
src/model_utils.jl 10 19.64%
src/loglikelihoods.jl 16 54.84%
Totals Coverage Status
Change from base Build 8111716086: -4.0%
Covered Lines: 2592
Relevant Lines: 3207

💛 - Coveralls

test/runtests.jl Outdated
@@ -76,7 +90,7 @@ include("test_util.jl")
DocMeta.setdocmeta!(
DynamicPPL,
:DocTestSetup,
:(using DynamicPPL, Distributions);
:(using DynamicPPL, Distributions, Accessors);
Copy link
Member Author

@sunxd3 sunxd3 Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I need to do this, the doc CI's failing can also be contributed to the same reason. Without it, got Undefined variable: Accessors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that's why? In some of the examples I see failing, there is no reference to Setfield / Accessors in the docstring that is being evaluated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the @varname macro changed? It seems like the generated expression might still keep a reference to Accessors in the model (which it shouldn't)?

Copy link
Member Author

@sunxd3 sunxd3 Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't https://github.com/TuringLang/DynamicPPL.jl/actions/runs/8658853355/job/23743552885?pr=585#step:5:167 fails because ERROR: UndefVarError: \Accessors\ not defined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the @varname macro changed? It seems like the generated expression might still keep a reference to Accessors in the model (which it shouldn't)?

I suspect this is the issue, but I haven't dig in.

@sunxd3
Copy link
Member Author

sunxd3 commented Apr 12, 2024

With TuringLang/AbstractPPL.jl#96, all the DynamicPPL tests pass

Comment on lines +27 to 43
[weakdeps]
ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4"
EnzymeCore = "f151be2c-9106-41f4-ab19-57ee4f262869"
ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210"
MCMCChains = "c7f686f2-ff18-58e9-bc7b-31028e88f75d"
ReverseDiff = "37e2e3b7-166d-5795-8a7a-e32c996b4267"
ZygoteRules = "700de1a5-db45-46bc-99cf-38207098b444"

[extensions]
DynamicPPLChainRulesCoreExt = ["ChainRulesCore"]
DynamicPPLEnzymeCoreExt = ["EnzymeCore"]
DynamicPPLForwardDiffExt = ["ForwardDiff"]
DynamicPPLMCMCChainsExt = ["MCMCChains"]
DynamicPPLReverseDiffExt = ["ReverseDiff"]
DynamicPPLZygoteRulesExt = ["ZygoteRules"]

[compat]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: this isn't something you've done manually but is something that occurs because you do ]instantiate or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happened automatic, not manually


function cross_val(
dataset::AbstractVector{<:Real};
dataset::Vector{<:Real};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

test/runtests.jl Outdated
@@ -76,7 +90,7 @@ include("test_util.jl")
DocMeta.setdocmeta!(
DynamicPPL,
:DocTestSetup,
:(using DynamicPPL, Distributions);
:(using DynamicPPL, Distributions, Accessors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that's why? In some of the examples I see failing, there is no reference to Setfield / Accessors in the docstring that is being evaluated?

test/runtests.jl Outdated
@@ -76,7 +90,7 @@ include("test_util.jl")
DocMeta.setdocmeta!(
DynamicPPL,
:DocTestSetup,
:(using DynamicPPL, Distributions);
:(using DynamicPPL, Distributions, Accessors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/runtests.jl Outdated
@@ -76,7 +90,7 @@ include("test_util.jl")
DocMeta.setdocmeta!(
DynamicPPL,
:DocTestSetup,
:(using DynamicPPL, Distributions);
:(using DynamicPPL, Distributions, Accessors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the @varname macro changed? It seems like the generated expression might still keep a reference to Accessors in the model (which it shouldn't)?

docs/make.jl Outdated Show resolved Hide resolved
@sunxd3
Copy link
Member Author

sunxd3 commented Apr 15, 2024

@torfjelde Actually, what do you mean by

It seems like the generated expression might still keep a reference to Accessors in the model (which it shouldn't)?

with Setfield

julia> @macroexpand @varname(x[1])
:((VarName){:x}((Setfield.compose)((Setfield.IndexLens)((1,)))))

so there are references to Setfield.

The issue seems to be that these references to Setfield gets compiled away, but with Accessors, they are not.
Consider

function demo_mv(::Type{TV}=Float64) where {TV}
     m = Vector{TV}(undef, 2)
     m[1] ~ Normal()
     m[2] ~ Normal()
     return m
 end

model = demo_mv();

code_typed(model.f, (DynamicPPL.Model, DynamicPPL.AbstractVarInfo, DynamicPPL.AbstractPPL.AbstractContext, typeof(Float64)))
With Accessors
CodeInfo(
1 ── %1   = $(Expr(:static_parameter, 1))::DataType%2   = Core.apply_type(Main.Vector, %1)::Type{Vector{_A}} where _A
│    %3   = (%2)(Main.undef, 2)::Vector%4   = Main.Accessors::Any%5   = Base.getproperty(%4, :compose)::Any%6   = Main.Accessors::Any%7   = Base.getproperty(%6, :IndexLens)::Any%8   = (%7)((1,))::Any%9   = (%5)(%8)::Any%10  = (VarName{:m})(%9)::VarName{:m}%11  = DynamicPPL.contextual_isassumption(__context__, %10)::Bool
└───        goto #9 if not %11
2 ── %13  = DynamicPPL.inargnames(%10, __model__)::Any%14  = !%13::Any
└───        goto #4 if not %14
3 ──        goto #7
4 ── %17  = DynamicPPL.inmissings(%10, __model__)::Any
└───        goto #6 if not %17
5 ──        goto #7
6 ── %20  = Base.getindex(%3, 1)::Any%21  = (%20 === Main.missing)::Bool
└───        goto #8
7 ┄─        nothing::Nothing
8 ┄─ %24  = φ (#7 => true, #6 => %21)::Bool
└───        goto #10
9 ──        nothing::Nothing
10%27  = φ (#8 => %24, #9 => false)::Bool%28  = DynamicPPL.contextual_isfixed(__context__, %10)::Bool
└───        goto #15 if not %28
11%30  = DynamicPPL.getfixed_nested(__context__, %10)::Any%31  = (isa)(%3, Vector{Any})::Bool
└───        goto #13 if not %31
12%33  = π (%3, Vector{Any})
│           Base.arrayset(true, %33, %30, 1)::Vector{Any}
└───        goto #14
13 ─        Base.setindex!(%3, %30, 1)::Any
└───        goto #14
14 ┄        goto #23
15 ─        goto #17 if not %27
16%40  = DynamicPPL.tilde_assume!!(__context__, $(QuoteNode(Normal{Float64}=0.0, σ=1.0))), %10, __varinfo__)::Tuple{Any, Any}%41  = Base.getfield(%40, 1)::Any%42  = Base.getfield(%40, 2)::Any%43  = Main.Accessors::Any%44  = Base.getproperty(%43, :set)::Any%45  = Main.BangBang::Any%46  = Base.getproperty(%45, :AccessorsImpl)::Any%47  = Base.getproperty(%46, :prefermutation)::Any%48  = Main.Accessors::Any%49  = Base.getproperty(%48, :opticcompose)::Any%50  = Main.Accessors::Any%51  = Base.getproperty(%50, :IndexLens)::Any%52  = (%51)((1,))::Any%53  = (%49)(%52)::Any%54  = (%47)(%53)::Any%55  = (%44)(%3, %54, %41)::Any
└───        goto #23
17%57  = DynamicPPL.inargnames(%10, __model__)::Any%58  = !%57::Any
└───        goto #22 if not %58
18%60  = DynamicPPL.getconditioned_nested(__context__, %10)::Any%61  = (isa)(%3, Vector{Any})::Bool
└───        goto #20 if not %61
19%63  = π (%3, Vector{Any})
│           Base.arrayset(true, %63, %60, 1)::Vector{Any}
└───        goto #21
20 ─        Base.setindex!(%3, %60, 1)::Any
└───        goto #21
21nothing::Nothing
22%69  = Base.getindex(%3, 1)::Any%70  = DynamicPPL.tilde_observe!!(__context__, $(QuoteNode(Normal{Float64}=0.0, σ=1.0))), %69, %10, __varinfo__)::Tuple{Any, Any}
└─── %71  = Base.getfield(%70, 2)::Any
23%72  = φ (#14 => %3, #16 => %55, #22 => %3)::Any%73  = φ (#14 => __varinfo__, #16 => %42, #22 => %71)::Any%74  = Main.Accessors::Any%75  = Base.getproperty(%74, :opticcompose)::Any%76  = Main.Accessors::Any%77  = Base.getproperty(%76, :IndexLens)::Any%78  = (%77)((2,))::Any%79  = (%75)(%78)::Any%80  = (VarName{:m})(%79)::VarName{:m}%81  = DynamicPPL.contextual_isassumption(__context__, %80)::Bool
└───        goto #31 if not %81
24%83  = DynamicPPL.inargnames(%80, __model__)::Any%84  = !%83::Any
└───        goto #26 if not %84
25 ─        goto #29
26%87  = DynamicPPL.inmissings(%80, __model__)::Any
└───        goto #28 if not %87
27 ─        goto #29
28%90  = Base.maybeview(%72, 2)::Any%91  = (%90 === Main.missing)::Bool
└───        goto #30
29nothing::Nothing
30%94  = φ (#29 => true, #28 => %91)::Bool
└───        goto #32
31nothing::Nothing
32%97  = φ (#30 => %94, #31 => false)::Bool%98  = DynamicPPL.contextual_isfixed(__context__, %80)::Bool
└───        goto #34 if not %98
33%100 = DynamicPPL.getfixed_nested(__context__, %80)::Any
│           Base.setindex!(%72, %100, 2)::Any
└───        goto #39
34 ─        goto #36 if not %97
35%104 = DynamicPPL.tilde_assume!!(__context__, $(QuoteNode(Normal{Float64}=0.0, σ=1.0))), %80, %73)::Tuple{Any, Any}%105 = Base.getfield(%104, 1)::Any%106 = Base.getfield(%104, 2)::Any%107 = Main.Accessors::Any%108 = Base.getproperty(%107, :set)::Any%109 = Main.BangBang::Any%110 = Base.getproperty(%109, :AccessorsImpl)::Any%111 = Base.getproperty(%110, :prefermutation)::Any%112 = Main.Accessors::Any%113 = Base.getproperty(%112, :opticcompose)::Any%114 = Main.Accessors::Any%115 = Base.getproperty(%114, :IndexLens)::Any%116 = (%115)((2,))::Any%117 = (%113)(%116)::Any%118 = (%111)(%117)::Any%119 = (%108)(%72, %118, %105)::Any
└───        goto #39
36%121 = DynamicPPL.inargnames(%80, __model__)::Any%122 = !%121::Any
└───        goto #38 if not %122
37%124 = DynamicPPL.getconditioned_nested(__context__, %80)::Any
└───        Base.setindex!(%72, %124, 2)::Any
38%126 = Base.maybeview(%72, 2)::Any%127 = DynamicPPL.tilde_observe!!(__context__, $(QuoteNode(Normal{Float64}=0.0, σ=1.0))), %126, %80, %73)::Tuple{Any, Any}
└─── %128 = Base.getfield(%127, 2)::Any
39%129 = φ (#33 => %72, #35 => %119, #38 => %72)::Any%130 = φ (#33 => %73, #35 => %106, #38 => %128)::Any%131 = Core.tuple(%129, %130)::Tuple{Any, Any}
└───        return %131
) => Tuple{Any, Any}
With Setfield
CodeInfo(
    1 ── %1   = $(Expr(:static_parameter, 1))::DataType%2   = Core.apply_type(Main.Vector, %1)::Type{Vector{_A}} where _A
    │    %3   = (%2)(Main.undef, 2)::Vector%4   = Main.Normal::Any%5   = (%4)()::Any%6   = (isa)(%5, NamedDist)::Bool
    └───        goto #3 if not %6
    2 ── %8   = π (%5, NamedDist)
    │    %9   = Base.getfield(%8, :name)::VarName
    └───        goto #6
    3 ──        goto #5 if not true
    4 ──        goto #6
    5 ──        Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
    └───        unreachable
    6 ┄─ %15  = φ (#2 => %9, #4 => $(QuoteNode(m[1])))::VarName%16  = (DynamicPPL.contextual_isassumption)(__context__, %15)::Bool
    └───        goto #14 if not %16
    7 ── %18  = (DynamicPPL.inargnames)(%15, __model__)::Any%19  = !%18::Any
    └───        goto #9 if not %19
    8 ──        goto #12
    9 ── %22  = (DynamicPPL.inmissings)(%15, __model__)::Any
    └───        goto #11 if not %22
    10 ─        goto #12
    11%25  = Base.getindex(%3, 1)::Any%26  = (%25 === Main.missing)::Bool
    └───        goto #13
    12nothing::Nothing
    13%29  = φ (#12 => true, #11 => %26)::Bool
    └───        goto #15
    14nothing::Nothing
    15%32  = φ (#13 => %29, #14 => false)::Bool%33  = (DynamicPPL.contextual_isfixed)(__context__, %15)::Bool
    └───        goto #20 if not %33
    16%35  = (DynamicPPL.getfixed_nested)(__context__, %15)::Any%36  = (isa)(%3, Vector{Any})::Bool
    └───        goto #18 if not %36
    17%38  = π (%3, Vector{Any})
    │           Base.arrayset(true, %38, %35, 1)::Vector{Any}
    └───        goto #19
    18 ─        Base.setindex!(%3, %35, 1)::Any
    └───        goto #19
    19 ┄        goto #42
    20 ─        goto #29 if not %32
    21%45  = (isa)(%5, AbstractArray{<:Distributions.Distribution})::Bool
    └───        goto #23 if not %45
    22%47  = π (%5, AbstractArray{<:Distributions.Distribution})
    └───        goto #28
    23%49  = (isa)(%5, Distributions.Distribution)::Bool
    └───        goto #25 if not %49
    24%51  = π (%5, Distributions.Distribution)
    └───        goto #28
    25 ─        goto #27 if not true
    26%54  = invoke DynamicPPL.check_tilde_rhs(%5::Any)::Any
    └───        goto #28
    27 ─        Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
    └───        unreachable
    28%58  = φ (#22 => %47, #24 => %51, #26 => %54)::Any%59  = (DynamicPPL.unwrap_right_vn)(%58, %15)::Tuple{Any, VarName}%60  = Core.getfield(%59, 1)::Any%61  = Core.getfield(%59, 2)::VarName%62  = (DynamicPPL.tilde_assume!!)(__context__, %60, %61, __varinfo__)::Tuple{Any, Any}%63  = Base.getfield(%62, 1)::Any%64  = Base.getfield(%62, 2)::Any%65  = (Setfield.set)(%3, $(QuoteNode(BangBang.SetfieldImpl.Lens!!{Setfield.IndexLens{Tuple{Int64}}}((@lens _[1])))), %63)::Any
    └───        goto #42
    29%67  = (DynamicPPL.inargnames)(%15, __model__)::Any%68  = !%67::Any
    └───        goto #34 if not %68
    30%70  = (DynamicPPL.getconditioned_nested)(__context__, %15)::Any%71  = (isa)(%3, Vector{Any})::Bool
    └───        goto #32 if not %71
    31%73  = π (%3, Vector{Any})
    │           Base.arrayset(true, %73, %70, 1)::Vector{Any}
    └───        goto #33
    32 ─        Base.setindex!(%3, %70, 1)::Any
    └───        goto #33
    33nothing::Nothing
    34%79  = (isa)(%5, AbstractArray{<:Distributions.Distribution})::Bool
    └───        goto #36 if not %79
    35%81  = π (%5, AbstractArray{<:Distributions.Distribution})
    └───        goto #41
    36%83  = (isa)(%5, Distributions.Distribution)::Bool
    └───        goto #38 if not %83
    37%85  = π (%5, Distributions.Distribution)
    └───        goto #41
    38 ─        goto #40 if not true
    39%88  = invoke DynamicPPL.check_tilde_rhs(%5::Any)::Any
    └───        goto #41
    40 ─        Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
    └───        unreachable
    41%92  = φ (#35 => %81, #37 => %85, #39 => %88)::Any%93  = Base.getindex(%3, 1)::Any%94  = (DynamicPPL.tilde_observe!!)(__context__, %92, %93, %15, __varinfo__)::Tuple{Any, Any}
    └─── %95  = Base.getfield(%94, 2)::Any
    42%96  = φ (#19 => %3, #28 => %65, #41 => %3)::Any%97  = φ (#19 => __varinfo__, #28 => %64, #41 => %95)::Any%98  = Main.Normal::Any%99  = (%98)()::Any%100 = (isa)(%99, NamedDist)::Bool
    └───        goto #44 if not %100
    43%102 = π (%99, NamedDist)
    │    %103 = Base.getfield(%102, :name)::VarName
    └───        goto #47
    44 ─        goto #46 if not true
    45 ─        goto #47
    46 ─        Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
    └───        unreachable
    47%109 = φ (#43 => %103, #45 => $(QuoteNode(m[2])))::VarName%110 = (DynamicPPL.contextual_isassumption)(__context__, %109)::Bool
    └───        goto #55 if not %110
    48%112 = (DynamicPPL.inargnames)(%109, __model__)::Any%113 = !%112::Any
    └───        goto #50 if not %113
    49 ─        goto #53
    50%116 = (DynamicPPL.inmissings)(%109, __model__)::Any
    └───        goto #52 if not %116
    51 ─        goto #53
    52%119 = (Base.maybeview)(%96, 2)::Any%120 = (%119 === Main.missing)::Bool
    └───        goto #54
    53nothing::Nothing
    54%123 = φ (#53 => true, #52 => %120)::Bool
    └───        goto #56
    55nothing::Nothing
    56%126 = φ (#54 => %123, #55 => false)::Bool%127 = (DynamicPPL.contextual_isfixed)(__context__, %109)::Bool
    └───        goto #58 if not %127
    57%129 = (DynamicPPL.getfixed_nested)(__context__, %109)::Any
    │           Base.setindex!(%96, %129, 2)::Any
    └───        goto #77
    58 ─        goto #67 if not %126
    59%133 = (isa)(%99, AbstractArray{<:Distributions.Distribution})::Bool
    └───        goto #61 if not %133
    60%135 = π (%99, AbstractArray{<:Distributions.Distribution})
    └───        goto #66
    61%137 = (isa)(%99, Distributions.Distribution)::Bool
    └───        goto #63 if not %137
    62%139 = π (%99, Distributions.Distribution)
    └───        goto #66
    63 ─        goto #65 if not true
    64%142 = invoke DynamicPPL.check_tilde_rhs(%99::Any)::Any
    └───        goto #66
    65 ─        Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
    └───        unreachable
    66%146 = φ (#60 => %135, #62 => %139, #64 => %142)::Any%147 = (DynamicPPL.unwrap_right_vn)(%146, %109)::Tuple{Any, VarName}%148 = Core.getfield(%147, 1)::Any%149 = Core.getfield(%147, 2)::VarName%150 = (DynamicPPL.tilde_assume!!)(__context__, %148, %149, %97)::Tuple{Any, Any}%151 = Base.getfield(%150, 1)::Any%152 = Base.getfield(%150, 2)::Any%153 = (Setfield.set)(%96, $(QuoteNode(BangBang.SetfieldImpl.Lens!!{Setfield.IndexLens{Tuple{Int64}}}((@lens _[2])))), %151)::Any
    └───        goto #77
    67%155 = (DynamicPPL.inargnames)(%109, __model__)::Any%156 = !%155::Any
    └───        goto #69 if not %156
    68%158 = (DynamicPPL.getconditioned_nested)(__context__, %109)::Any
    └───        Base.setindex!(%96, %158, 2)::Any
    69%160 = (isa)(%99, AbstractArray{<:Distributions.Distribution})::Bool
    └───        goto #71 if not %160
    70%162 = π (%99, AbstractArray{<:Distributions.Distribution})
    └───        goto #76
    71%164 = (isa)(%99, Distributions.Distribution)::Bool
    └───        goto #73 if not %164
    72%166 = π (%99, Distributions.Distribution)
    └───        goto #76
    73 ─        goto #75 if not true
    74%169 = invoke DynamicPPL.check_tilde_rhs(%99::Any)::Any
    └───        goto #76
    75 ─        Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
    └───        unreachable
    76%173 = φ (#70 => %162, #72 => %166, #74 => %169)::Any%174 = (Base.maybeview)(%96, 2)::Any%175 = (DynamicPPL.tilde_observe!!)(__context__, %173, %174, %109, %97)::Tuple{Any, Any}
    └─── %176 = Base.getfield(%175, 2)::Any
    77%177 = φ (#57 => %96, #66 => %153, #76 => %96)::Any%178 = φ (#57 => %97, #66 => %152, #76 => %176)::Any%179 = Core.tuple(%177, %178)::Tuple{Any, Any}
    └───        return %179
    ) => Tuple{Any, Any}

@yebai @devmotion

@sunxd3
Copy link
Member Author

sunxd3 commented Apr 15, 2024

Also, all the tests pass when Accessors is exposed in Main

@torfjelde
Copy link
Member

torfjelde commented Apr 15, 2024

The issue seems to be that these references to Setfield gets compiled away, but with Accessors, they are not.

Compilation should really not affect this issue; if there's an unresolved reference, there's no way to know what to compile.

I'm wondering if something somewhere is not interpolated correctly.

E.g. consider the following:

julia> module A
           id(x) = x
       end
Main.A

julia> module B
           using ..A: A
           macro a(x)
               :(A.id($x))
           end
           macro b(x)
               :($(A.id)($x))
           end
           macro c(x)
               :($(esc(:id))($x))
           end
       end
Main.B

julia> @macroexpand B.@a 1
:((Main.B.A).id(1))

julia> @macroexpand B.@b 1
:((Main.A.id)(1))

julia> @macroexpand B.@c 1 # This will not work
:(id(1))

So could there be some unfortunate interaction with esc and some references somewhere that occur now but now before?

@sunxd3
Copy link
Member Author

sunxd3 commented Apr 18, 2024

turns out the reason is lot more stupid, fixed at TuringLang/AbstractPPL.jl#97.

@sunxd3
Copy link
Member Author

sunxd3 commented Apr 18, 2024

Should be okay, but probably wait for #586

@yebai yebai marked this pull request as ready for review April 18, 2024 14:49
Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sunxd3 -- happy to merge from my side!

@yebai yebai requested a review from torfjelde April 18, 2024 19:31
@yebai
Copy link
Member

yebai commented Apr 18, 2024

@torfjelde, can you give this a second look so we can merge it?

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great stuff @sunxd3 :) Probably not the most fun PR to complete 😬 So creds to pushing through it 🎉

@yebai yebai enabled auto-merge April 18, 2024 21:10
@yebai yebai added this pull request to the merge queue Apr 18, 2024
Merged via the queue into master with commit 00604db Apr 18, 2024
11 of 12 checks passed
@yebai yebai deleted the sunxd/adapt_accesssors branch April 18, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants