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

Fix esc bug with kernel auxiliary arguments in kernel DSL. #461

Merged
merged 8 commits into from
Apr 14, 2022

Conversation

femtomc
Copy link
Contributor

@femtomc femtomc commented Mar 31, 2022

No description provided.

@femtomc
Copy link
Contributor Author

femtomc commented Mar 31, 2022

One other comment: postwalk was actually walking into tr ~ k(tr, args...) twice, applying Gen.reversal two times.

Would like to add tests for the DSL somehow.

Copy link
Member

@ztangent ztangent left a comment

Choose a reason for hiding this comment

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

I think this is mostly making the right fixes, but we have to be careful about how escaping the final Expr affects name resolution for names and references we're injecting into the generated code.

For example, if you escape a block of code which contains a line like Gen.reversal(...), Gen.reversal may not always end up pointing to the right function, depending on what Gen refers to in the context where the macro is called. To avoid this, we should either avoid escaping lines like that, or if easier, replace Gen.reversal with $(GlobalRef(Gen, :reversal)).

src/inference/kernel_dsl.jl Outdated Show resolved Hide resolved
Gen.is_custom_primitive_kernel($(k)) || error("first function is not a custom primitive kernel")
Gen.is_custom_primitive_kernel($(l)) || error("second function is not a custom primitive kernel")
Gen.reversal(::typeof($(k))) = $(l)
Gen.reversal(::typeof($(l))) = $(k)
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why the escs are being removed here. I think they should stay...? What's the result of calling @macroexpand on the version without the escs?

src/inference/kernel_dsl.jl Outdated Show resolved Hide resolved
src/inference/kernel_dsl.jl Show resolved Hide resolved
end
Gen.check_is_kernel(::typeof($(esc(f)))) = true
Gen.check_is_kernel(::typeof($(f))) = true
Copy link
Member

Choose a reason for hiding this comment

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

Replace Gen.check_is_kernel with GlobalRef.

src/inference/kernel_dsl.jl Outdated Show resolved Hide resolved
src/inference/kernel_dsl.jl Show resolved Hide resolved
src/inference/kernel_dsl.jl Show resolved Hide resolved
…ent for both forward and reverse kernel definitions.
…ent for both forward and reverse kernel definitions.
@femtomc
Copy link
Contributor Author

femtomc commented Apr 14, 2022

Also, here's a reference expansion:

quote
    function myk(tr::Trace; var"##check#282" = false, var"##observations#283" = EmptyChoiceMap())
        var"##check#282" && Gen.check_is_kernel(mh)
        tr = (mh(tr, select(:a), var"##check#282" = var"##check#282", var"##observations#283" = var"##observations#283"))[1]
        var"##check#282" && Gen.check_observations(get_choices(tr), var"##observations#283")
        metadata = nothing
        (tr, metadata)
    end
    Gen.check_is_kernel(::typeof(myk)) = true
    function var"##reverse_kernel#284"(tr::Trace; var"##check#282" = false, var"##observations#283" = EmptyChoiceMap())
        var"##check#282" && Gen.check_is_kernel(Gen.reversal(mh))
        tr = ((Gen.reversal(mh))(tr, select(:a), var"##check#282" = var"##check#282", var"##observations#283" = var"##observations#283"))[1]
        var"##check#282" && Gen.check_observations(get_choices(tr), var"##observations#283")
        metadata = nothing
        (tr, metadata)
    end
    Gen.check_is_kernel(::typeof(var"##reverse_kernel#284")) = true
    Gen.reversal(::typeof(myk)) = var"##reverse_kernel#284"
    Gen.reversal(::typeof(var"##reverse_kernel#284")) = myk
end

@femtomc
Copy link
Contributor Author

femtomc commented Apr 14, 2022

Ah, @ztangent -- we can't gensym the keyword arguments, because we assume check and observations throughout the Gen inference library (for MCMC kernels) :(

@femtomc femtomc requested a review from ztangent April 14, 2022 05:06
@femtomc
Copy link
Contributor Author

femtomc commented Apr 14, 2022

Okay, I removed outermost esc -- now only escaping kernel + reversal names, and macro expansion takes care of gensyming the argument names.

Copy link
Member

@ztangent ztangent left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for doing this :)

@ztangent ztangent changed the title Fix esc bug with kernel auxiliary arguments in kernel DSL. Refactor -- move esc to final Expr. Fix esc bug with kernel auxiliary arguments in kernel DSL. Apr 14, 2022
@ztangent ztangent merged commit 7b6a884 into master Apr 14, 2022
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.

None yet

3 participants