[Merged by Bors] - Don't "compile" inputs of macros

[torfjelde]

At the moment we will actually call generate_mainbody! on inputs to macros inside the model, e.g. in a model @mymacro x ~ Normal() will actually result in code @mymacro $(generate_mainbody!(:(x ~ Normal()))) (or something, you get the idea).

IMO, this shouldn't be done for the following reasons:

1. Breaks with what you'd expect in Julia, IMO, which is that a macro eats the "raw" code.
2. Means that if we want to do stuff like @reparam from Reparameterizations #220 (and a bunch of others, see [Merged by Bors] - Small simplification of compiler #221 for a small list of possibilities), we need touch the compiler rather than just make a small macro that will perform transformations after the compiler has done it's job (referring to DynamicPPL compiler here).
3. If the user wants to use a macro on some variables, but they want the actual variable rather than messing around with the sample-statement, they can just separate it into two lines, e.g. x ~ Normal(); @mymacro ....

Also, to be completely honest, for the longest time I've just assumed that I'm not even allowed to do @mymacro x ~ Normal() and have things work 😅 I bet a lot of people have the same impression by default (though this might of course just not be true:) )

[devmotion]

I'm not sure if there's really a common/standard convention regarding the execution of nested macros in Julia. There are so many different ways to interpolate things, protect them from expansion, etc. and so many different use cases...

Maybe instead of just protecting macro calls (apart from __dot__) one could call macroexpand(__module__, ex; recursive=false) in such a case and expand it before the DynamicPPL compiler creates the model?

[devmotion]

It seems in this case one does not even have to distinguish between __dot__ and other macros anymore, one would just always expand one layer of the macro?

[torfjelde]

Maybe instead of just protecting macro calls (apart from dot) one could call macroexpand(module, ex; recursive=false) in such a case and expand it before the DynamicPPL compiler creates the model?

But isn't the "intuitive" thing that the model just doesn't touch it? I guess, why would we call macroexpand rather than just waiting with macro expansion until after the model is done (and have the model not touch the internal code)?

I'm just weary of "manually" touching the expansion of the macros I suppose. Feels like that is prone to error or misunderstanding for the user.

[devmotion]

But isn't the "intuitive" thing that the model just doesn't touch it? I guess, why would we call macroexpand rather than just waiting with macro expansion until after the model is done (and have the model not touch the internal code)?

OK, maybe I misunderstood your initial post then 😄 I think expanding @mymacro before performing the DynamicPPL rewriting would actually be the more intuitive thing - that's e.g. also how @. works:

julia> macro mymacro(ex)
           return 42
       end

julia> @macroexpand @. @mymacro(x + y)
42

[devmotion]

Ah, the example was bad, it is actually the other way around 😄

julia> macro mymacro(ex)
           @show ex
           return esc(:(x - y))
       end

julia> @macroexpand @. @mymacro(x + y)
ex = :((+).(x, y))
:(x - y)

[devmotion]

But.... This example shows that the current behaviour (that applies generate_body! to the macro arguments) is actually in line with how @. behaves.

[torfjelde]

Oh well that's my intuition out the window!

BUT @. doesn't actually "manually" expand the code though, like we would do. It just goes "Oh, I don't recongize this as something I can dot, so I'm just going to dot it's args instead" (https://github.com/JuliaLang/julia/blob/13caa8abf2320ea7e6f7255adf3477cbd6096e87/base/broadcast.jl#L1259-L1271)

[torfjelde]

Just clarify: I'm not against expanding the macro before the model does it's thing. It would actually be pretty dope since you can then generate expressions with ~ in it! 🎉 Making the @reparam macro for example would then be completely trivial:)

I just don't have a lot of experience with using macroexpand in this way so I'm in no position to evaluate whether it can cause issues 😅

Also: why do recursive=false? Curious.

[torfjelde]

I can think of why it should cause issues though. The more I think about it, the more I like your suggestion of expanding first..

[devmotion]

As I said above, IMO it really depends on the macro you use whether it is more intuitive to expand inner layers first or not. Since the @model macro rewrites such large blocks of the code, I think it could be reasonable to expand inner stuff first and let users generate x ~ dist blocks and other parts of the model instead of keeping it until the end (since then you would have to manually generate the observe/assume calls). And it seems one could still avoid expansion by protecting it with $?

recursive=false is maybe not useful, initially I was just worried that expansion might be too invasive.

[torfjelde]

And it seems one could still avoid expansion by protecting it with $?

That's a good point. I like this. Currently trying out macroexpand 👍

[torfjelde]

Made the change. Now e.g. the following works:

julia> using DynamicPPL, Distributions

julia> macro reparam(f, expr)
           L, R = DynamicPPL.getargs_tilde(expr)

           res = quote
               tmp ~ $R
               $L = $f(tmp)
           end
           return esc(res)
       end
@reparam (macro with 1 method)

julia> @model function demo()
           @reparam x -> 0 x ~ Normal()

           return x
       end
demo (generic function with 1 method)

julia> demo()()
0

julia> ex = @macroexpand @model function demo()
           @reparam x -> 0 x ~ Normal()

           return x
       end;

julia> ex |> Base.remove_linenums!
quote
    $(Expr(:meta, :doc))
    function demo(; )
        var"##evaluator#286" = ((_rng::Random.AbstractRNG, _model::Model, _varinfo::AbstractVarInfo, _sampler::AbstractMCMC.AbstractSampler, _context::DynamicPPL.AbstractContext)->begin
                    begin
                        begin
                            begin
                                var"##tmpright#282" = Normal()
                                var"##tmpright#282" isa Union{Distribution, AbstractVector{<:Distribution}} || throw(ArgumentError("Right-hand side of a ~ must be subtype of Distribution or a vector of Distributions."))
                                var"##vn#284" = tmp
                                var"##inds#285" = ()
                                tmp = (DynamicPPL.tilde_assume)(_rng, _context, _sampler, var"##tmpright#282", var"##vn#284", var"##inds#285", _varinfo)
                            end
                            x = ((x->begin
                                        0
                                    end))(tmp)
                        end
                        return x
                    end
                end)
        return (Model)(:demo, var"##evaluator#286", NamedTuple(), NamedTuple())
    end
end

(Of course it's a stupid example, but you get the idea.)

[torfjelde]

Tests also pass locally 👍

[torfjelde]

What do you think about the updated version @devmotion ?

[devmotion]

Nice, I like this change. Can you add some tests?

[devmotion]

Maybe it would make sense to be a bit more consistent and group the internal variables together, i.e., make them e.g. the first two arguments? E.g.

Suggested change

	esc(model(expr, __source__, warn, __module__))
	esc(model(__module__, __source__, expr, warn))

(as __module__ is also the first argument of macroexpand)

[torfjelde]

Haha, I literally made the change of grouping them together (but didn't move them to the first argument, but I agree with this too).

But opinions on what to do with generate_mainbody!, etc.? Personally I don't like switching ordering of arguments, e.g .__module__ being before warn in model but then warn comes before __module__ in generate_mainbody!. Should I make mod the first argument to generate_mainbody! too maybe?

[devmotion]

Ha I thought about this as well. I didn't suggest it because one would have to change quite many lines. But I agree that it would be more consistent if mod would be the first argument there as well 👍

[torfjelde]

Haha, but sweet I'll do that then 👍

[devmotion]

For the suggestion above this would be changed to

Suggested change

	function model(expr, linenumbernode, warn, mod)
	function model(mod, linenumbernode, expr, warn)

[torfjelde]

Going out for a bit, but I'll get back to this later (and the other PRs) 👍

[devmotion]

I guess we should also add some tests for the macro expansion and for interpolated macros (just to be sure that it works). E.g. something like

macro mymodel()
    return esc(:(x ~ Normal()))
end

@model function demo()
    @mymodel()
end

@test haskey(VarInfo(demo()), @varname(x))

and

macro mymodel()
    return esc(:(return 42))
end

@model function demo()
    x ~ Normal()
    $(@mymodel())
end

@test demo()() == 42

[torfjelde]

Sorry, hadn't gotten around to adding tests yet; but added the ones you suggested just now:)

[torfjelde]

Also, I must have been particular sleepy when I made this PR: I just noticed that I have named the branc tor/DONT-expand-macro-inputs, which is exactly the opposite of what I was trying to achieve 😅

[torfjelde]

Ah crap I see I forgot to move the mod argument as we discussed. It think I actually idd, but realized I was on the wrong branch and stashed so that I could apply to this one later.
Omw to bed so I'll get that done tomorrow 👍

[devmotion]

Ah this was a bad idea since it should give the same result, regardless if the macro is expanded or not. Maybe the following captures better what we want to test:

Suggested change

	macro mymodel()
	return esc(:(return 42))
	end
	@model function demo()
	x ~ Normal()
	$(@mymodel())
	end
	struct MyModelStruct{T}
	x::T
	end
	Base.:~(x, y::MyModelStruct) = y.x
	macro mymodel(ex)
	# check if expression was modified by the DynamicPPL "compiler"
	if ex == :(y ~ Uniform())
	return :(4 ~ MyModelStruct(42))
	else
	return :(return -1)
	end
	end
	@model function demo()
	$(@mymodel(y ~ Uniform()))
	end

Then the test will fail if

• the DynamicPPL compiler expands y ~ Uniform() before expanding the macro (returns -1)
• @mymodel is expanded before the DynamicPPL compiler has transformed the whole @model expression (error since MyModelStruct is not a distribution, and hence the tilde_observe errors)

[devmotion]

Maybe the struct (and/or the macros) have to be defined at the top level, I am not sure...

[devmotion]

Looks good, just a minor thing (whitespace instead of tabs). It's a breaking change since the model syntax changes but I think the current behaviour is a bug and therefore the non-breaking version bump is fine 👍

[devmotion]

White space instead of tabs:

Suggested change

	return esc(:(x ~ Normal()))
	else
	return esc(:(z ~ Exponential()))
	end
	return esc(:(x ~ Normal()))
	else
	return esc(:(z ~ Exponential()))
	end

[devmotion]

Ah seems I approved too quickly - it would be good to improve the test of protected macros as well (maybe along the lines of https://github.com/TuringLang/DynamicPPL.jl/pull/222/files#r606757078).

[torfjelde]

Ehh not certain what you're linking there 😕 It shows me src/compiler.jl

[devmotion]

Somehow it's still not correct?

Suggested change

	return esc(:(x ~ Normal()))
	else
	return esc(:(z ~ Exponential()))
	end
	return esc(:(x ~ Normal()))
	else
	return esc(:(z ~ Exponential()))
	end

[torfjelde]

Should be good now 👍

[devmotion]

Ehh not certain what you're linking there

Ah, sorry, probably I copied the wrong link. It seems you updated the second test as well, so all good 👍

[devmotion]

LGTM

[torfjelde]

bors r+

[bors]

Build failed:

• test (1, ubuntu-latest, x64, 2)

[torfjelde]

bors r+

[bors]

Pull request successfully merged into master.

Build succeeded:

• test (1.3, macOS-latest, x64, 1)
• test (1.3, macOS-latest, x64, 2)
• test (1.3, ubuntu-latest, x64, 1)
• test (1.3, ubuntu-latest, x64, 2)
• test (1.3, ubuntu-latest, x86, 1)
• test (1.3, ubuntu-latest, x86, 2)
• test (1.3, windows-latest, x64, 1)
• test (1.3, windows-latest, x64, 2)
• test (1, macOS-latest, x64, 1)
• test (1, macOS-latest, x64, 2)
• test (1, ubuntu-latest, x64, 1)
• test (1, ubuntu-latest, x64, 2)
• test (1, ubuntu-latest, x86, 1)
• test (1, ubuntu-latest, x86, 2)
• test (1, windows-latest, x64, 1)
• test (1, windows-latest, x64, 2)
• Turing.jl