You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
```julia
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).
Co-authored-by: David Widmann <[email protected]>
ind !==nothing||throw(ArgumentError("Please make sure type parameters are properly used. Every `Type{T}` argument need to have `T` in the a `where` clause"))
128
-
end
129
-
Expr(:kw, :($T::Type{<:$S}), Tval)
130
-
else
131
-
arg
104
+
105
+
# Extract the positional and keyword arguments from the model definition.
106
+
allargs =vcat(modeldef[:args], modeldef[:kwargs])
107
+
108
+
# Split the argument expressions and the default values.
109
+
allargs_exprs_defaults =map(allargs) do arg
110
+
MacroTools.@match arg begin
111
+
(x_ = val_) => (x, val)
112
+
x_ => (x, NO_DEFAULT)
113
+
end
114
+
end
115
+
116
+
# Extract the expressions of the arguments, without default values.
117
+
allargs_exprs =first.(allargs_exprs_defaults)
118
+
119
+
# Extract the names of the arguments.
120
+
allargs_syms =map(allargs_exprs_defaults) do (arg, _)
121
+
MacroTools.@match arg begin
122
+
(::Type{T_}) | (name_::Type{T_}) => T
123
+
name_::T_=> name
124
+
x_ => x
132
125
end
133
126
end
134
-
args_nt =to_namedtuple_expr(arg_syms)
135
127
128
+
# Build named tuple expression of the argument symbols and variables of the same name.
0 commit comments