Always call r_to_py S3 method when converting objects from Python to R #299
Conversation
|
Given that I think that having static references to these Rcpp objects may be dangerous -- the destructor will attempt to unprotect the associated SEXP, which may no longer exist at shutdown time. I think leaving them as non-static would be okay. |
|
Okay, I got rid of the static. I ran the tests against keras and tensorflow and both had no issues. I think it would be good to also do some diagnostics along these lines:
|
|
Beyond that. I believe that I've gotten the memory semantics right here but it's definitely worth another pair of eyes tracking back the implications of things. Would also be good to just empirically confirm that we aren't leaking any memory with the new implementation. |
|
If there were a memory leak, I would expect code like this to cause the R session's memory usage to blow up: library(reticulate)
df <- data.frame(alpha = 1:1E3)
py_run_string('def identity(x): return x')
main <- import_main()
repeat main$identity(df)All seems well in this case -- memory usage remains stable. |
|
Okay, I'm running right now with |
|
|
||
| // call the R version and hold the return value in a PyObjectRef (SEXP wrapper) | ||
| // this object will be released when the function returns | ||
| PyObjectRef ref(r_to_py_fn(x, convert)); |
There was a problem hiding this comment.
Just to make sure I understand the semantics, PyObjectRef itself is an R environment containing the associated Python wrapper object. A C finalizer is set on the Python object when this object is constructed, and that C finalizer will call Py_DecRef() when the parent R environment is cleaned up. To ensure that the object then lives beyond that cleanup, we need to explicitly increase the reference count (as is done below).
There was a problem hiding this comment.
Yes, that's exactly correct (note that py_to_r_cpp effectively does the same thing, incrementing the reference count before returning any Python object).
R CMD check with gctorture() is still running, no problems yet....
There was a problem hiding this comment.
Okay, gctorture() succeeded. If you are good with the code changes I'll merge....
|
LGTM! I think we're ready to merge. I also ran a revdepcheck and all the downstream dependencies look good -- no breakages due to this change. |
|
Just a wee comment to say thanks for this: things get a little easier in altair. We can now remove the alt$Chart(r_to_py(vega_data$cars()))I was getting ready to file an issue, but no need! cc @AliciaSchep |
Addresses #298
@kevinushey Could you review this please?
I'm wondering if the
staticreferences I introduced to the R reticulate environment andr_to_py()functions could cause any problems? (e.g. being feed at the end of a process lifetime when R is already gone?).The other issue is ensuring that we have all of the memory semantics right (i.e. we won't either leak objects or leave an object without a reference causing a crash). This is about the lowest level change we could make in reticulate so has the potential to wreak havoc if it's not done correctly!