Add py_func() to wrap R function in a Python function with correct signature

[terrytangyuan]

This is to address #141, basically a generalized version of as_model_fn in tfestimators.

Note that this function is experimental and not exported since currently only simple function is supported (e.g. no ... or other complicated cases).

[jjallaire]

Awesome! Let me take a closer look at this and see if there is a lower level way to do this that doesn't rely on the string interpolation (e.g. some of the approaches described here: https://stackoverflow.com/questions/11291242/python-dynamically-create-function-at-runtime)

[jjallaire]

After doing some further research, I think that you have taken the only feasible approach here.

I think it would be okay to rename wrap_fn to py_funct and to export it. @kevinushey What do you think?

[terrytangyuan]

Thanks for taking the time to do that! I've done some experiments using other approaches but no luck (either not fully working or hard to customize). I agree that string interpolation is not ideal but I think it should support most of the use cases already and easy to customize. There's definitely a lot of space for improvements and additional test cases.

[kevinushey]

Is there some well-defined set of functions that we can / should be able to correctly wrap? I think the following examples might fail in the current implementation:

function(a = 1, b) {}
function(x = NA) {}
function(a.b) {}
function(a = "abc") {}
function(a = list()) {}
function(a = NULL) {}

Do any of these need to be handled?

[kevinushey]

To check if an argument is missing, you want to use the (rather esoteric) construct:

identical(sigs[[k]], quote(expr =))

[kevinushey]

For things that aren't just literals, R will preserve the call object used in the formals, e.g.

> f <- function(a = c(1, 2, 3)) {}
> class(formals(f)$a)
[1] "call"

I'm guessing that R's language objects may not marshal as well in these cases. Do we need to explicitly evaluate these defaults before attempting to convert them?

[terrytangyuan]

Yes, I'll fix this

[kevinushey]

FWIW, rather than growing the signature over time, you could string-ify each formal independently and then paste when you're done, e.g.

strings <- lapply(sigs, function(x) { <convert to Python string equivalent> }
paste(strings, collapse = ", ")

[terrytangyuan]

@kevinushey Thanks. I'll try fix those cases.

• function(x = NA) {} - let's support this later since What should NA be converted to in Python? #197 needs to be addressed first. There could be NAs in a list, etc which can get more complicated so I'd prefer to rely on r_to_py for those cases.
• function(a.b) {} - @kevinushey What is the goal of this one?

[kevinushey]

That just reflects the fact that R argument names can contain a ., which I think is not possible in Python. In practice I guess that implies that we should avoid using such argument names with anything that we would want to wrap for Python? (I suppose it's not relevant)

[terrytangyuan]

@kevinushey Yea I would expect users to use more normal argument names. Should we stop() if we find such a case? Do you have a generalized regex in mind for these stop cases？

[kevinushey]

Something like [a-zA-Z_][a-zA-Z0-9_]*?

[terrytangyuan]

@kevinushey I think you meant [a-zA-Z_].[a-zA-Z0-9_]* with the .?
Also, we cannot support function(a = 1, b) {} for similar reason. Python requires default arguments before non-default arguments. We also need to capture that in the regex which gets more complicated...

Perhaps we should just point out in the docs that the function signature has to be in Python style that meets Python's requirements?

[terrytangyuan]

@kevinushey Or wrap the whole thing in a tryCatch

[jjallaire]

This can definitely tolerate some imperfections and failure modes because it is going to be on opt-in (by default you will get the equivalent of function(...) marshaled). The main motivation for this is Python libraries that "type check" callbacks by looking for a specific signature. By definition these signatures will not contain esoteric Python-incompatible constructs.

[kevinushey]

Oh, I see -- you meant a regex for capturing the bad case? You could just use e.g. regepxr(".", fixed = TRUE) to catch that case. But I agree that we can just point to the docs and use tryCatch().

[kevinushey]

Given the limited scope I think the main thing to fix for the PR is the handling of some more common R default arguments (NULL, character strings, atomic vectors, maybe lists?)

[jjallaire]

Remember also that callback functions being type-checked by Python libraries aren't that likely to have default arguments (since these are library not user level interfaces)

…

On Thu, Mar 29, 2018 at 2:27 PM, Kevin Ushey ***@***.***> wrote: Given the limited scope I think the main thing to fix for the PR is the handling of some more common R default arguments (NULL, character strings, atomic vectors, maybe lists?) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGXxzFFvLGpvZ50y6RzuGsAbFtWmm2Iks5tjVHJgaJpZM4S7aWk> .

[terrytangyuan]

@jjallaire That's a good point actually. I just pushed some fixes for those additional test cases.

Any suggested better name for this function?

[terrytangyuan]

@kevinushey I was trying to apply your suggested R syntactical enhancements but for some reason I was hitting issues on those so I'll probably keep this as it is for now. I think the rest of your comments should have been addressed already except that we need a better name for the function + export it.

[jjallaire]

I think it should be name py_func()

[terrytangyuan]

@jjallaire Done renaming.
@kevinushey I just applied your suggested syntactical improvements.

Let me know if there's anything else I missed.

[jjallaire]

I don't think the tryCatch with fallback to the default behavior is really helping us here. The user thinks they got a complete Python signature but instead they end up getting the default ... style signature. Why is this better than an error? (I'm presuming that the only reason the user is calling py_func is because they require the correct signature for use with a library that checks callbacks -- silently giving them an unexpected return value seems bad).

[terrytangyuan]

Makes sense. I’ll change this

[jjallaire]

Let's add a NEWS.md entry for this. Let's also add a brief section to this doc: https://github.com/rstudio/reticulate/blob/master/vignettes/calling_python.Rmd