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

refactor py_str macro to allow passing custom global/locals #777

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marius311
Copy link
Contributor

@marius311 marius311 commented May 12, 2020

This should have no change to functionality, but the new @_py_str macro can be used by other functions to pass custom globals/locals dicts into Python expression while still getting the benefit of @py_str's variable interpolation.

@_py_str is kept a macro rather than function because it makes downstream macro hygene a bit easier.

I'm using this here JuliaPy/pyjulia#381, where this can maybe be discussed more.

src/pyeval.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

Could you clarify why this should be a macro?

src/pyeval.jl Outdated Show resolved Hide resolved
src/pyeval.jl Outdated
esc(macroexpand(__module__, :(PyCall.@_py_str($m, $m, $code, $(options...)))))
end

macro _py_str(pyglobals, pylocals, code, options...)
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to use a function for this, it should take a named tuple (__module__=__module__, __source__=__source__) as the first argument so that all the information available within a usual macro is available. I think it's better to make it a single first argument rather than two arguments because future versions of julia may introduce some more information within a macro definition but it'd be bad to change the calling convention of this function.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should use options rather than options....

src/pyeval.jl Outdated Show resolved Hide resolved
@marius311
Copy link
Contributor Author

marius311 commented May 15, 2020

Could you clarify why this should be a macro?

So it stems from the fact that inside @prepare_for_pyjulia_call we do a complicated recursive walk through the whole expression to transform it. Its really nice to just be able to stick an esc around the whole transformed thing at the end here, otherwise you have to think (hard) about where to stick in esc's throughout that walk. Maybe there's a simple way, but all my playing with it it got really complex.

Given that you want to esc the whole thing, then whatever expression you get from calling into PyCall can't have bare symbols like PyObject or pyeval_ in it, they need to be turned into PyCall.PyObject and PyCall.pyeval_. You can do this by modifing things on the PyCall side, but it seems easier to just make @_py_str a macro and then hygene does this for you. That's basically my motivation.

That said, I realize now all the macroexpand(__module__, ... stuff is unnecessary so I'll get rid of it in both PRs.

@@ -202,32 +198,37 @@ pasted into the Python code. This allows you to evaluate code
where the code itself is generated by a Julia expression.
"""
macro py_str(code, options...)
m = :(pynamespace($__module__))
fname = String(__source__.file)
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...))))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this call to esc necessary, since there are esc calls within @_py_str?

Indeed, the call to esc here seems like it might cause problems, because it prevents the names pylocals, pyglobals, and ret from being hygienized.

Copy link
Contributor Author

@marius311 marius311 May 15, 2020

Choose a reason for hiding this comment

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

No, pylocals etc... get hygenized by @_py_str, the esc here makes it so the interpolated variables don't also get hygenized, e.g. with the code in this PR:

julia> @macroexpand py"1+$i"
quote
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:229 =#
    (var"#18#pyglobals", var"#19#pylocals") = (PyCall.pynamespace(Main), PyCall.pynamespace(Main))
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:230 =#
    begin
        var"#19#pylocals"["__julia_localvar_2_1"] = PyCall.PyObject(i)
    end
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:231 =#
    var"#20#ret" = (PyAny)(PyCall.pyeval_((string)("1+__julia_localvar_2_1"), var"#18#pyglobals", var"#19#pylocals", 258, "none"))
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:232 =#
    begin
        PyCall.delete!(var"#19#pylocals", "__julia_localvar_2_1")
    end
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:233 =#
    var"#20#ret"
end

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I should've asked this before but why is insert_pyevals run at run-time? It looks like this doesn't depend on the run-time value of globals and locals (or maybe I'm missing something)? If we expand everything at macro-expansion time, perhaps esc of @py_str here and @prepare_for_pyjulia_call there can be removed? Then maybe _py_str doesn't have to be a macro?

@@ -202,32 +198,37 @@ pasted into the Python code. This allows you to evaluate code
where the code itself is generated by a Julia expression.
"""
macro py_str(code, options...)
m = :(pynamespace($__module__))
fname = String(__source__.file)
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...))))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...))))
esc(:($PyCall.@_py_str($m, $m, $fname, $code, $(options...))))

I think you need this?

I think we better test py"" with something like

baremodule TestPyStr
using PyCall: @py_str
pystr(x) = py"$$x"
end
@test TestPyStr.pystr("1+1") == 2

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.

3 participants