[Merged by Bors] - BREAKING Make evaluator a method of the model function #316

devmotion · 2021-08-24T08:16:48Z

This PR implements the idea in https://github.com/TuringLang/DynamicPPL.jl/pull/314/files#r693968700. All DynamicPPL tests pass, I only had to update the doctests (since the number of methods that are created by @model changed from 1 to 2).

This PR is breaking: it breaks deserialization of models since Julia can't deserialize generic functions (see below).

It allows to do the following:

julia> using DynamicPPL, Distributions

julia> @macroexpand @model demo() = x ~ Normal()
quote
    function demo(__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext; )
        #= REPL[4]:1 =#
        begin
            var"##vn#257" = (VarName){:x}()
            var"##inds#258" = ()
            var"##isassumption#259" = begin
                    let var"##vn#260" = (VarName){:x}()
                        if (DynamicPPL.contextual_isassumption)(__context__, var"##vn#260")
                            if !((DynamicPPL.inargnames)(var"##vn#260", __model__)) || (DynamicPPL.inmissings)(var"##vn#260", __model__)
                                true
                            else
                                x === missing
                            end
                        else
                            false
                        end
                    end
                end
            if var"##isassumption#259"
                x = (DynamicPPL.tilde_assume!)(__context__, (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(Normal()), var"##vn#257")..., var"##inds#258", __varinfo__)
            else
                if !((DynamicPPL.inargnames)(var"##vn#257", __model__))
                    x = (DynamicPPL.getvalue_nested)(__context__, var"##vn#257")
                end
                (DynamicPPL.tilde_observe!)(__context__, (DynamicPPL.check_tilde_rhs)(Normal()), x, var"##vn#257", var"##inds#258", __varinfo__)
            end
        end
    end
    begin
        $(Expr(:meta, :doc))
        function demo(; )
            #= REPL[4]:1 =#
            return (Model)(:demo, demo, NamedTuple(), NamedTuple())
        end
    end
end

julia> @model demo() = x ~ Normal()
demo (generic function with 2 methods)

julia> f(x) = false
f (generic function with 1 method)

julia> f(::Model{typeof(demo)}) = true
f (generic function with 2 methods)

julia> f(demo())
true

julia> demo() isa Model{typeof(demo)}
true

devmotion · 2021-08-24T08:27:28Z

Turing tests fail with some weird precompilation error locally:

julia> using Turing
[ Info: Precompiling Turing [fce5fe82-541a-59a6-adf8-730c64b5f9a0]
ERROR: LoadError: LoadError: cannot assign a value to variable Core.Core from module Turing

But since the same error occurs with the latest release of DynamicPPL I guess it's not caused by this PR.

devmotion · 2021-08-24T08:28:00Z

bors try

bors · 2021-08-24T08:37:04Z

try

Build failed:

Turing.jl

devmotion · 2021-08-24T08:44:10Z

At least the errors can be reproduced with CI 🙂 At least one warning seems to be caused by a bug introduced in the latest release of DistributionsAD, I'll open a PR.

devmotion · 2021-08-24T09:32:49Z

The errors can be reproduced with the master branch as well: #317 So I am sure the approach here does not cause them.

torfjelde · 2021-08-24T09:55:43Z

Wait, how does this work for multiple models named demo? Won't you end up with several implementations of demo(__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext; ) ?

devmotion · 2021-08-24T10:01:49Z

No, this was my initial thought but if you define e.g. demo(x) you end up with demo(x) and demo(::Model, ::AbstractVarInfo, ::AbstractContext, x), so the evaluator is only redefined if you redefine a model.

torfjelde · 2021-08-24T10:06:10Z

Ooooh true! But this does mean that we can't actually do typeof(demo) to distinguish two different demo models, right? 😕

Btw, I do quite like the idea of just having the evaluator available like this irregardless of trying to support dispatching on Model:)

devmotion · 2021-08-24T10:16:00Z

Ooooh true! But this does mean that we can't actually do typeof(demo) to distinguish two different demo models, right? confused

No. Personally I don't think this is too limiting since you can still dispatch on the arguments as well. I.e., in this case you just have to include more type parameters in your dispatch.

torfjelde · 2021-08-24T10:23:12Z

Good point 👍 I'm quite happy with this approach:)

The only thing I'm worried about is serialization: deserializing a Model now assumes the existence of demo, no? While in the current implementation it's self-contained since serializing Model involves serializing the evaluator too.

devmotion · 2021-08-24T10:40:17Z

Serialization should not be an issue - Model still contains a reference to the evaluator. At least all serialization tests still pass.

torfjelde · 2021-08-24T10:53:44Z

It is though. Try running the following two blocks in two different REPLs.

REPL 1:

julia> using DynamicPPL, Distributions, Serialization

julia> @model demo() = x ~ Normal()
demo (generic function with 2 methods)

julia> m = demo();

julia> serialize("model.jls", m)

REPL 2:

julia> using DynamicPPL, Distributions, Serialization

julia> m = deserialize("model.jls")
ERROR: UndefVarError: #demo not defined
[...]

This works just fine in the current release because the evaluator will be serialized together with the model.

devmotion · 2021-08-24T11:45:24Z

Oh yeah, that's not surprising actually, generic functions have to be redefined in Julia and are not serialized/deserialized. Only closures are. It worked before automatically since Model stored only a closure. See, e.g., TuringLang/Turing.jl#1091 (comment).

devmotion · 2021-08-24T11:46:03Z

So you have to load the model and redefine the function with @model 🤷

torfjelde · 2021-08-24T11:48:51Z

Oh yeah, that's not surprising actually, generic functions have to be redefined in Julia and are not serialized/deserialized. Only closures are.

Yeah exactly, that was my point:)

So you have to load the model and redefine the function with @model

Eeehh and is that something we want to be the case? 😕

devmotion · 2021-08-24T12:20:35Z

Eeehh and is that something we want to be the case? confused

I don't think it's a design goal but a limitation of the Julia serializer that we can live with. Usually you know which models you work with and then you can also redefine/rewrite the @model definition if you deserialize it. And if you define the model in a package (or use such a model), then you just have to load the package. This seems reasonable.

In any case, IMO one should not use serialization for persistent storage since there are no guarantees that one can deserialize it with a different Julia or package version. JLSO.jl solves these issues but is still limited by the Julia serializer.

torfjelde · 2021-08-24T12:43:10Z

I don't think it's a design goal but a limitation of the Julia serializer that we can live with.

Hmm, not sure I agree with this given that we there's a way to circumvent it, i.e. the current approach.

Usually you know which models you work with and then you can also redefine/rewrite the @model definition if you deserialize it.

Really not the case in modelling projects. You usually have a bunch of models but a single post-processing/visualize/analysis script which makes few assumptions about the model (maybe that all the models have some common target variable or something). Being able to just load the model used to produce a chain is then very useful.

And if you define the model in a package (or use such a model), then you just have to load the package.

I like this though. But this also works with the current anonymous functions. Though it def ends up being nicer with the approach in this PR.

In any case, IMO one should not use serialization for persistent storage since there are no guarantees that one can deserialize it with a different Julia or package version. JLSO.jl solves these issues but is still limited by the Julia serializer.

Sure but this is AFAIK an irrelevant issue for exactly the reason you mention, i.e. that JLSO.jl uses the Julia serializer under the hood anyways.

devmotion · 2021-08-24T12:51:52Z

Personally I think this one disadvantage (that you have to make sure the function is defined if you deserialize a model) is completely outweighed by the advantage of removing hidden closures and being able to access the evaluator and dispatch on models without any hacks or heuristics and without having to instantiate a model. Even more so since the disadvantage is caused by a limitation of the Julia serializer itself.

Moreover, the serialization tests in https://github.com/TuringLang/DynamicPPL.jl/blob/master/test/serialization.jl all pass. These tests were broken in previous design iterations and one reason (surely not the only one) for sticking with the current design was that it fixed the pmap and serialize/deserialize issues that are tested there.

torfjelde · 2021-08-24T13:00:40Z

Personally I think this one disadvantage (that you have to make sure the function is defined if you deserialize a model) is completely outweighed by the advantage of removing hidden closures and being able to access the evaluator and dispatch on models without any hacks or heuristics and without having to instantiate a model.

I'm on the fence atm, i.e. not sure what I prefer. I def agree that this is a much nicer approach and if it wasn't for the serialization issue I would have been 100% on board. My issue is that from a user-perspective we're unnecessarily (again, from the user perspective) removing a feature. I don't like that 😕

Even more so since the disadvantage is caused by a limitation of the Julia serializer itself.

I don't understand why this is very relevant though. Telling a user "we can do this, but really Julia should be able to handle this other case so we're going to not support the feature and wait until maybe Julia can handle our approach instead" seems a bit weird, no? 😅 Usually we take the opposite approach: hack around Julia's limitations, and then once they're no longer there we drop the hacks.

torfjelde · 2021-08-24T13:06:00Z

How about we leave it for a couple of days, and have a chat about it? Maybe we join the Thursday meeting with @yebai and @phipsgabler ? Should get some input from the rest of the team too.

And I really want to emphasize that I'm not just trying to be a contrarian here; I genuinely think this is the better approach, but I also really don't like that it hurts the user a bit 😕

devmotion · 2021-08-24T13:11:17Z

We should just provide a proper way to store models instead or with some improved serialization 🤷 The issue is not newly introduced by this PR, already currently it exists for anyone who tries to serialize a Model that is created manually or in another way without @model. Unfortunately, I think it is not sufficient to just serialize a Model where the evaluator is replaced with an equivalent closure. Then the deserialized model would not dispatch correctly anymore and you would lose the traits etc.

IMO this PR does not only degrade user experience and remove features but also adds features, e.g., now you can dispatch on your model without having to instantiate it, with an arguably intuitive design (you can just dispatch on Model{typeof(demo)}). Of course, it's a matter of personal taste and use if one thinks the advantages outweigh the disadvantages 🤷

torfjelde · 2021-08-24T13:33:32Z

We should just provide a proper way to store models instead or with some improved serialization

If we had this, I'm 100% on board:)

The issue is not newly introduced by this PR, already currently it exists for anyone who tries to serialize a Model that is created manually or in another way without @model.

Sure, but nobody does that 😅

Unfortunately, I think it is not sufficient to just serialize a Model where the evaluator is replaced with an equivalent closure. Then the deserialized model would not dispatch correctly anymore and you would lose the traits etc.

Exactly, you lose the ability to make the traits persistent between serialization. But given that model traits is not a thing yet I'm more inclined towards the route which provides a feature with some limitations but without any breaking changes (this PR is breaking since the serialization behavior now changes).

IMO this PR does not only degrade user experience and remove features but also adds features, e.g., now you can dispatch on your model without having to instantiate it, with an arguably intuitive design (you can just dispatch on Model{typeof(demo)}).

Yeah, yeah of course! I'm not saying this PR doesn't have any benefits, haha. I'm just saying that it also comes with one annoying drawback, and because of this I don't want to rush it. As of right now, I do think this PR will be supersede #314 , but I want to give it a bit more thought.

devmotion · 2021-08-24T14:34:50Z

Yes, no need to rush it. I just thought this morning that the idea could actually work and wanted to put up a PR. It turned out much easier and cleaner than I had expected 🙂

And ideally we would improve and fix how models are saved first to avoid any regression.

phipsgabler · 2021-08-24T15:43:41Z

I think I love this. If it can be made to work. I just need to understand the serialization issue further... give me some time :D

torfjelde · 2021-08-24T16:35:13Z

Btw, this PR "solve" (we shouldn't be serializing/deserializing using different versions of a package anyways, but this at least makes it more likely to work):

tor@tor-Prestige-15-A10SC:~/Projects/public/DynamicPPL.jl$ GROUP=DynamicPPL julia --project
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.2 (2021-07-14)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> module A
           using DynamicPPL, Distributions
           @model demo() = x ~ Normal()
       end
Main.A

julia> m = A.demo()
DynamicPPL.Model{Main.A.var"#1#2", (), (), (), Tuple{}, Tuple{}, DynamicPPL.DefaultContext}(:demo, Main.A.var"#1#2"(), NamedTuple(), NamedTuple(), DynamicPPL.DefaultContext())

julia> using Serialization

julia> serialize("model.jls", m)

julia> 
tor@tor-Prestige-15-A10SC:~/Projects/public/DynamicPPL.jl$ GROUP=DynamicPPL julia --project
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.2 (2021-07-14)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using Serialization

julia> module A
           using DynamicPPL, Distributions
           @model notdemo() = 1
           @model demo() = x ~ Normal()
       end
Main.A

julia> m = deserialize("model.jls")
DynamicPPL.Model{Main.A.var"#1#2", (), (), (), Tuple{}, Tuple{}, DynamicPPL.DefaultContext}(:demo, Main.A.var"#1#2"(), NamedTuple(), NamedTuple(), DynamicPPL.DefaultContext())

julia> m()
1

On this PR, the deserialized version will still point to evaluator of A.demo rather than A.notdemo :)

torfjelde

I've given it some though and this is my response:

BUT could you maybe add the tests from the other PR or something similar? Just so we ensure that we continue to support dispatching on model.

devmotion · 2021-08-26T23:50:16Z

bors try

torfjelde

Dope:)

bors · 2021-08-27T00:37:43Z

try

Build succeeded:

torfjelde · 2021-08-27T07:53:31Z

Dope:) We also need a version bump btw. And this would be breaking, right? As we mentioned before, it can break serialized models, etc.

devmotion · 2021-08-27T08:05:04Z

Yes, I did not want to rush it and hence did not plan to merge it before the meeting. I thought maybe @yebai wants to have a look at the PR as well.

phipsgabler · 2021-08-27T08:58:30Z

Didn't we have a dev branch for that? :D

devmotion · 2021-08-27T09:20:40Z

We deleted it a long time ago, mostly since it did not encourage to release new features timely, dev diverged from the master branch and syncing in a more or less automatic way created a lot of useless commits and was messy in general.

devmotion · 2021-08-27T15:04:28Z

@torfjelde I updated the version number to 0.15.0.

yebai

Many thanks @devmotion and @torfjelde - looks good to me!

devmotion · 2021-08-27T15:51:57Z

bors r+

@macroexpand

This PR implements the idea in https://github.com/TuringLang/DynamicPPL.jl/pull/314/files#r693968700. All DynamicPPL tests pass, I only had to update the doctests (since the number of methods that are created by `@model` changed from 1 to 2). **This PR is breaking:** it breaks deserialization of models since Julia can't deserialize generic functions (see below). It allows to do the following: ```julia julia> using DynamicPPL, Distributions julia> @macroexpand @model demo() = x ~ Normal() quote function demo(__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext; ) #= REPL[4]:1 =# begin var"##vn#257" = (VarName){:x}() var"##inds#258" = () var"##isassumption#259" = begin let var"##vn#260" = (VarName){:x}() if (DynamicPPL.contextual_isassumption)(__context__, var"##vn#260") if !((DynamicPPL.inargnames)(var"##vn#260", __model__)) || (DynamicPPL.inmissings)(var"##vn#260", __model__) true else x === missing end else false end end end if var"##isassumption#259" x = (DynamicPPL.tilde_assume!)(__context__, (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(Normal()), var"##vn#257")..., var"##inds#258", __varinfo__) else if !((DynamicPPL.inargnames)(var"##vn#257", __model__)) x = (DynamicPPL.getvalue_nested)(__context__, var"##vn#257") end (DynamicPPL.tilde_observe!)(__context__, (DynamicPPL.check_tilde_rhs)(Normal()), x, var"##vn#257", var"##inds#258", __varinfo__) end end end begin $(Expr(:meta, :doc)) function demo(; ) #= REPL[4]:1 =# return (Model)(:demo, demo, NamedTuple(), NamedTuple()) end end end julia> @model demo() = x ~ Normal() demo (generic function with 2 methods) julia> f(x) = false f (generic function with 1 method) julia> f(::Model{typeof(demo)}) = true f (generic function with 2 methods) julia> f(demo()) true julia> demo() isa Model{typeof(demo)} true ``` Co-authored-by: David Widmann <[email protected]>

bors · 2021-08-27T16:02:31Z

Build failed:

test (1.3, ubuntu-latest, x64, 1)

devmotion · 2021-08-27T16:05:56Z

bors r+

@macroexpand

This PR implements the idea in https://github.com/TuringLang/DynamicPPL.jl/pull/314/files#r693968700. All DynamicPPL tests pass, I only had to update the doctests (since the number of methods that are created by `@model` changed from 1 to 2). **This PR is breaking:** it breaks deserialization of models since Julia can't deserialize generic functions (see below). It allows to do the following: ```julia julia> using DynamicPPL, Distributions julia> @macroexpand @model demo() = x ~ Normal() quote function demo(__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext; ) #= REPL[4]:1 =# begin var"##vn#257" = (VarName){:x}() var"##inds#258" = () var"##isassumption#259" = begin let var"##vn#260" = (VarName){:x}() if (DynamicPPL.contextual_isassumption)(__context__, var"##vn#260") if !((DynamicPPL.inargnames)(var"##vn#260", __model__)) || (DynamicPPL.inmissings)(var"##vn#260", __model__) true else x === missing end else false end end end if var"##isassumption#259" x = (DynamicPPL.tilde_assume!)(__context__, (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(Normal()), var"##vn#257")..., var"##inds#258", __varinfo__) else if !((DynamicPPL.inargnames)(var"##vn#257", __model__)) x = (DynamicPPL.getvalue_nested)(__context__, var"##vn#257") end (DynamicPPL.tilde_observe!)(__context__, (DynamicPPL.check_tilde_rhs)(Normal()), x, var"##vn#257", var"##inds#258", __varinfo__) end end end begin $(Expr(:meta, :doc)) function demo(; ) #= REPL[4]:1 =# return (Model)(:demo, demo, NamedTuple(), NamedTuple()) end end end julia> @model demo() = x ~ Normal() demo (generic function with 2 methods) julia> f(x) = false f (generic function with 1 method) julia> f(::Model{typeof(demo)}) = true f (generic function with 2 methods) julia> f(demo()) true julia> demo() isa Model{typeof(demo)} true ``` Co-authored-by: David Widmann <[email protected]>

bors · 2021-08-27T16:30:51Z

Pull request successfully merged into master.

Build succeeded:

devmotion added 2 commits August 24, 2021 10:02

Make evaluator a method of the model function

bc52790

Fix doctests

c67922d

bors bot added a commit that referenced this pull request Aug 24, 2021

Try #316:

93ca9dd

devmotion mentioned this pull request Aug 24, 2021

[Merged by Bors] - Reduce CI #317

Closed

torfjelde requested changes Aug 26, 2021

View reviewed changes

devmotion and others added 2 commits August 27, 2021 01:49

Add tests

3a2ed9f

Merge branch 'master' into dw/evaluatordispatch

a558e2e

bors bot added a commit that referenced this pull request Aug 26, 2021

Try #316:

a836e18

torfjelde approved these changes Aug 27, 2021

View reviewed changes

Update Project.toml

78e76d2

devmotion changed the title ~~Make evaluator a method of the model function~~ **BREAKING** Make evaluator a method of the model function Aug 27, 2021

Merge branch 'master' into dw/evaluatordispatch

af8fb16

yebai approved these changes Aug 27, 2021

View reviewed changes

Update Project.toml

08b1988

bors bot changed the title **BREAKING** Make evaluator a method of the model function [Merged by Bors] - **BREAKING** Make evaluator a method of the model function Aug 27, 2021

bors bot closed this Aug 27, 2021

bors bot deleted the dw/evaluatordispatch branch August 27, 2021 16:30

torfjelde mentioned this pull request Sep 7, 2021

Model-dispatching and Experimental submodule #314

Closed

[Merged by Bors] - **BREAKING** Make evaluator a method of the model function #316

[Merged by Bors] - **BREAKING** Make evaluator a method of the model function #316

Uh oh!

Conversation

devmotion commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

bors bot commented Aug 24, 2021

try

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devmotion commented Aug 24, 2021

Uh oh!

phipsgabler commented Aug 24, 2021

Uh oh!

torfjelde commented Aug 24, 2021

Uh oh!

torfjelde left a comment

Choose a reason for hiding this comment

Uh oh!

devmotion commented Aug 26, 2021

Uh oh!

torfjelde left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Aug 27, 2021

try

Uh oh!

torfjelde commented Aug 27, 2021

Uh oh!

devmotion commented Aug 27, 2021

Uh oh!

[Merged by Bors] - BREAKING Make evaluator a method of the model function #316

[Merged by Bors] - BREAKING Make evaluator a method of the model function #316

devmotion commented Aug 24, 2021 •

edited

Loading

devmotion commented Aug 24, 2021 •

edited

Loading

torfjelde commented Aug 24, 2021 •

edited

Loading

torfjelde commented Aug 24, 2021 •

edited

Loading

torfjelde commented Aug 24, 2021 •

edited

Loading

torfjelde commented Aug 24, 2021 •

edited

Loading

torfjelde commented Aug 24, 2021 •

edited

Loading

torfjelde commented Aug 24, 2021 •

edited

Loading