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

rmd_reader: Make set_initoptions() compatible with rpy2 v3 #1135

Merged
merged 1 commit into from
Feb 11, 2021
Merged

rmd_reader: Make set_initoptions() compatible with rpy2 v3 #1135

merged 1 commit into from
Feb 11, 2021

Conversation

stephenfeagin
Copy link
Contributor

Fixes #1132.

rpy2 changed its API for 3.0, and moved set_initoptions() from rpy2.rinterface to rpy2.rinterface_lib.embedded. It also changed the argument from a tuple of bytes to a tuple of strings. This PR tries to run it as rpy2 v2, and if the method lookup fails, it tries the rpy2 v3 setup. I've tested this with both rpy2==3.0.2 and rpy2==2.9.5.

Try to run rpy2.rinterface.set_initoptions(). If it fails, try rpy2 v3 syntax, in rpy2.rinterface_lib.embedded.
@jranke
Copy link
Contributor

jranke commented Feb 10, 2021

I just upgraded to the upcoming Debian bullseye, and rpy2 v2 does not install any more. First I deinstalled the current Debian package of python3-rpy2 3.4.2. Then pip install "rpy2<3" fails with (only giving the most relevant part of the output):

...

 ./rpy/rinterface/_rpy_device.c: In function ‘_GrDev_close’:
    ./rpy/rinterface/_rpy_device.c:175:7: error: ‘_Py_DEC_REFTOTAL’ undeclared (first use in this function); did you mean ‘_Py_DECREF’?

...

For me this means that I would really like to see rmd_reader to be compatible with current rpy2, which is the purpose of this PR. Note that the README should be updated as well, as it currently says that rpy2 v2 is needed and recommends the abovementioned pip install command.

@jranke
Copy link
Contributor

jranke commented Feb 10, 2021

Ah, yes, and I I can confirm that the PR solves the problem. I checked out the PR and rebased it on master. Should I update the Readme and requirements.txt and create a new PR, or what would be your preferred procedure here?

@justinmayer
Copy link
Member

@jranke: Thank you for reminding me about this. I think the best thing to do here is to merge this PR, after which you can submit a new PR to address the other points you raised (README, requirements.txt, etc.).

@stephane-caron: Very sorry for the delay in merging your contribution, which is much appreciated! 😅

@justinmayer justinmayer merged commit d1dc806 into getpelican:master Feb 11, 2021
@stephenfeagin stephenfeagin deleted the rmd_reader branch February 11, 2021 16:26
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.

rmd_reader fails with rpy2 >=3.0
3 participants