[Merged by Bors] - Remove ModelGen

[devmotion]

This PR removes ModelGen completely. The main motivation for it were the issues that @itsdfish and I experienced when working with multiple processes. The following MWE

using Distributed
addprocs(4)
@everywhere using DynamicPPL

@everywhere @model model() = begin end

pmap(x -> model(), 1:4)

fails intermittently

if not all of these [@model] evaluations generate same evaluator and generator functions of the same name (i.e., these var"###evaluator#253" and var"###generator#254" functions). I assume one could maybe provoke the error by defining another model first on the main process before calling @Everywhere @model ....

(copied from the discussion on Slack)

With the changes in this PR, @model model() = ... generates only a single function model on all workers, and hence there are no issues anymore with the names of the generators and evaluators. The evaluator is created as a closure inside of model, which can be serialized and deserialized properly by Julia. So far I haven't been able to reproduce the issues above with this PR.

The only user-facing change of the removal of ModelGen (apart from that one never has to construct it, which simplifies the docs and the example @denainjs asked about) is that the logprob macro now requires to specify model = m where m = model() instead of model = model (since that's just a regular function from which the default arguments etc of the resulting model can't be extracted). It feels slightly weird that the evaluation is not based "exactly" on the specified Model instance but that the other parts of logprob modify it (which was the reason I guess for using the model generator before here), but on the other hand this weird behaviour already exists when specifying logprob with a Chains object. (BTW I'm not sure if we should actually use string macros here, maybe regular functions would be nicer.)

Additionally, I assume (though haven't tested it) that getting rid of the separate evaluator and generator functions will not only simplify serialization and deserialization when working with multiple processes but also when saving models and chains (see e.g. TuringLang/Turing.jl#1091).

[itsdfish]

Thank you for looking into this! It looks like this change will simplify several things.

[phipsgabler]

How cool is that! 🍰

[yebai]

There are some really nice improvements in this PR.

I'm not sure if we should actually use string macros here, maybe regular functions would be nicer.)

I agree the string macro is a bit strange and would be nice to consider a regular function replacement.

Additionally, I assume (though haven't tested it) that getting rid of the separate evaluator and generator functions will not only simplify serialization and deserialization when working with multiple processes but also when saving models and chains (see e.g. TuringLang/Turing.jl#1091).

Would be great to experiment with these ideas and try to tidy up the compiler API/design.

[phipsgabler]

I'm not sure if we should actually use string macros here, maybe regular functions would be nicer.)

I agree the string macro is a bit strange and would be nice to consider a regular function replacement.

I have never used it, but also wondered about that choice. We could use some kind of formula objects, like GLM does, although "formula" is a strange name and we shouldn't reuse the tilde here. And unfortunately, | binds stronger than assignment and comma, so @prob(x = 1 | y = 2) parses as (= x (= (| 1 y) 2)).

On the other hand, wouldn't something like

density(model, s = 1.0, m = 1.0, given = (;x = 1.0), chain = c)

suffice to analyze which variables go where and perform the according call?

[devmotion]

Regarding the precedence issue, one could use, e.g., @logprob (x = 1) | (y = 3, z = 24) which seems to parse correctly.

In fact that would be trivial to implement it seems, since it could just be lowered to the logprob function in

DynamicPPL.jl/src/prob_macro.jl

Line 24 in 275ccc8

function logprob(ex1, ex2)

. It seems the logprob"..." macro just wraps the LHS and RHS of | as tuples and then calls logprob, so it's already like the density suggestion with the model and chain being part of the RHS.

IMO it might be more natural to keep the model and the chain that is used in these computations separate from the LHS and RHS of | since it can only be part of the RHS anyways. Similar to the density suggestion, one could just require the model and/or the chain to be the first argument of density/logprob. That would also avoid issues if a variable is called chain or model. Maybe it would also resolve the current precedence issues (at least there were some discussions and issues a while ago IIRC) about what to do when both a chain with a saved model and a model are specified since we could define the function signature and provided dispatches at will. In the same way the @logprob macro could be extended with additional arguments that are not part of the tuples.

[phipsgabler]

Regarding the precedence issue, one could use, e.g., @logprob (x = 1) | (y = 3, z = 24) which seems to parse correctly.

Sure, but that would 1) force the user to use parentheses in a place where "semantically" the form without parentheses should be equally valid, and 2) will hit people with the subtle distinction between @macro (foo) and @macro(foo). That's why I didn't suggest that form.

Now I get why this was made a string macro :D

In fact that would be trivial to implement it seems, since it could just be lowered to the logprob function [...]. It seems the logprob"..." macro just wraps the LHS and RHS of | as tuples and then calls logprob, so it's already like the density suggestion with the model and chain being part of the RHS.

IMO it might be more natural to keep the model and the chain that is used in these computations separate from the LHS and RHS of | since it can only be part of the RHS anyways. Similar to the density suggestion, one could just require the model and/or the chain to be the first argument of density/logprob.

Exactly my thought.

New suggestion for sugar: using ; for conditioning, make @pr construct a "probability formula", somewhat like the mathematical notation Pr(p1(x), p2(y); z), where we deal with predicates of x and y, and pass this to logprob and logdensity functions, just that we always have to be explicit about names:

logprob(model, @pr(x == 1; alpha = 2))
logprob(chain, @pr y > 0)

(And yes, I realize that this suffers from the same @macro (foo) problem :( )

One thing I'm just not sure about is how rigidly we should separate densities and probabilities. That's why I called the function density. I'd rather not further propagate the confusion many people have distinguishing between them (and likelihoods) by calling them the same.

[devmotion]

I guess the main motivation for a macro over just having a function logdensity(model, ::Tuple, ::Tuple) would be that it might be more convenient for specifying the predicates?

[devmotion]

BTW (since I just worked with it) the macro syntax with brackets and | is also used in the pattern matching in MacroTools: https://mikeinnes.github.io/MacroTools.jl/stable/pattern-matching/#Unions-1

[devmotion]

Ah I just noticed that the latest commits broke the MWE above - one really has to use anonymous functions it seems. Unfortunately that means that currently we either have to add ExprTools as additional dependency or reimplement the logic in MacroTools master (I asked about a new release but I don't know how soon there'll be one). I guess it would be fine to copy the parts of MacroTools that we need right now and remove them if a new release is available?

[yebai]

Sounds good.

[devmotion]

This is ready for a second round of reviewing @yebai @phipsgabler

I removed ExprTools and copied the logic for combining anonymous functions from MacroTools (hopefully there's a release soon, then it can be removed again; I would also prefer actually if they merge ExprTools into MacroTools). I also simplified the code for extracting the arguments and their default values and used MacroTools.@match for increasing the readability when parsing expressions. If we stick with MacroTools for now, #121 might not be needed anymore (or only in a reduced form).

[phipsgabler]

Very nice! I couldn't find any flaws in reading the code. build_model_info got really slim, I agree that #121 might have become superfluous with this.

[yebai]

bors r+

[bors]

Build failed:

• test (1, ubuntu-latest, x64)

[yebai]

bors r+

[bors]

Pull request successfully merged into dev.

Build succeeded:

• continuous-integration/travis-ci/push
• test (1.0, windows-latest, x64)
• test (1, macOS-latest, x64)
• test (1.0, macOS-latest, x64)
• test (1.0, ubuntu-latest, x64)
• test (1, ubuntu-latest, x86)
• test (1, ubuntu-latest, x64)
• test (1.0, ubuntu-latest, x86)
• test (1, windows-latest, x86)
• test (1, windows-latest, x64)
• test (1.0, windows-latest, x86)