Opened 3 years ago
Closed 3 years ago
#26791 closed defect (fixed)
R help text differs on some systems
Reported by:  ghtimokau  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  packages: standard  Keywords:  rpy2 R 
Cc:  charpent, embray, fbissey, jdemeyer  Merged in:  
Authors:  Timo Kaufmann  Reviewers:  Emmanuel Charpentier 
Report Upstream:  N/A  Work issues:  
Branch:  bd0f8f2 (Commits, GitHub, GitLab)  Commit:  bd0f8f2b77fd2853d63890f6d5b5aa6479c8caef 
Dependencies:  Stopgaps: 
Description (last modified by )
See https://groups.google.com/d/msg/sagerelease/MXSnQJh8EE/2vNChwf_CQAJ.
It looks like rpy2 picks up the systems R.
When a symbol has been overloaded, R's help
doesn't pick the right symbol.
Change History (46)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
 Branch set to u/ghtimokau/rpy2buildtypo
 Commit set to c25df8dc460e3d810ea47f0c2340f6b3252ff746
 Status changed from new to needs_review
New commits:
c25df8d  Fix rpy2 build script typo

comment:4 Changed 3 years ago by
That's a case where you want a revision bump in the package version. We want everyone to reinstall this.
comment:5 in reply to: ↑ 3 ; followup: ↓ 10 Changed 3 years ago by
Replying to ghtimokau:
@charpent please test.
Shall do. But I have to upgrade my local installation first. Don't expect results before tomorrow morning...
Note that the version described in my my report (done on another machine !) used Sage's R (3.4.4) rather than system's R (3.5.1, as packaged by Debian).
comment:6 Changed 3 years ago by
 Commit changed from c25df8dc460e3d810ea47f0c2340f6b3252ff746 to 2af44d2cf377bf9509d9d5a4d9f044c99ba55759
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2af44d2  Fix rpy2 build script typo

comment:7 followup: ↓ 9 Changed 3 years ago by
Right, added the package bump.
How do you know which version rpy2 actually used?
New commits:
2af44d2  Fix rpy2 build script typo

comment:8 Changed 3 years ago by
 Commit changed from 2af44d2cf377bf9509d9d5a4d9f044c99ba55759 to aeb8d30f215a426b32fe096a5f22a3a25755b106
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
aeb8d30  Fix rpy2 build script typo

comment:9 in reply to: ↑ 7 ; followup: ↓ 15 Changed 3 years ago by
Replying to ghtimokau:
Right, added the package bump.
How do you know which version rpy2 actually used?
Do you mean which version of R
was rpy2 using? Or which version of rpy2 was installed?
comment:10 in reply to: ↑ 5 ; followup: ↓ 16 Changed 3 years ago by
Replying to charpent:
Replying to ghtimokau:
@charpent please test.
Shall do. But I have to upgrade my local installation first. Don't expect results before tomorrow morning...
Note that the version described in my my report (done on another machine !) used Sage's R (3.4.4) rather than system's R (3.5.1, as packaged by Debian).
It turns out that I can't check out the branch related to this ticket (see sagedevel).
But manually fixing the typo in a local branch doesn fix the problem :
sage f rpy2 sage t long warnlong 47.5 src/sage/interfaces/r.py
gives the same result :
 sage t long warnlong 47.5 src/sage/interfaces/r.py # 3 doctests failed 
Visual inspection shows the same docstring differences as before.
PS : I saw a new update of this ticket while I was writing this : I still can't trac checkout
this ticket. Now we have two problems 8(.
New commits:
aeb8d30  Fix rpy2 build script typo

comment:11 followup: ↓ 12 Changed 3 years ago by
 Status changed from needs_review to needs_work
Not solved (see above)...
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 3 years ago by
Replying to charpent:
Not solved (see above)...
Question: Did you update some R packages or install new ones on top of the sage installation?
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 3 years ago by
Replying to fbissey:
Replying to charpent:
Not solved (see above)...
Question: Did you update some R packages or install new ones on top of the sage installation?
Nope.
BTW : your patches do not seem to have an updated checksum. I *think* that this is normal if the package itself remains the same... But shouldn't build/pkgs/rpy2/packageversion.txt
get bumped ?
comment:14 in reply to: ↑ 13 Changed 3 years ago by
Replying to charpent:
BTW : your patches do not seem to have an updated checksum. I *think* that this is normal if the package itself remains the same... But shouldn't
build/pkgs/rpy2/packageversion.txt
get bumped ?
I already advised Timo on that and the latest version of the branch has the bump.
comment:15 in reply to: ↑ 9 Changed 3 years ago by
Replying to fbissey:
Replying to ghtimokau:
Right, added the package bump.
How do you know which version rpy2 actually used?
Do you mean which version of
R
was rpy2 using? Or which version of rpy2 was installed?
The first one.
comment:16 in reply to: ↑ 10 ; followup: ↓ 17 Changed 3 years ago by
Replying to charpent:
Replying to charpent:
Replying to ghtimokau:
@charpent please test.
Shall do. But I have to upgrade my local installation first. Don't expect results before tomorrow morning...
Note that the version described in my my report (done on another machine !) used Sage's R (3.4.4) rather than system's R (3.5.1, as packaged by Debian).
It turns out that I can't check out the branch related to this ticket (see sagedevel).
But manually fixing the typo in a local branch doesn fix the problem :
sage f rpy2 sage t long warnlong 47.5 src/sage/interfaces/r.py
Did you reinstall rpy2
between those commands? I think f
just removes the package and without reinstalling it before testing, god knows what happens.
Could you also post the result of
./sage R version # version of sage's R R version # system R ./sage c 'import rpy2; print(rpy2.__version__)' # sages rpy version ./sage c 'from rpy2.rinterface import R_VERSION_BUILD; print(R_VERSION_BUILD)' # the R sages rpy was build with
comment:17 in reply to: ↑ 16 ; followup: ↓ 18 Changed 3 years ago by
Replying to ghtimokau:
Replying to charpent:
Replying to charpent:
Replying to ghtimokau:
@charpent please test.
Shall do. But I have to upgrade my local installation first. Don't expect results before tomorrow morning...
Note that the version described in my my report (done on another machine !) used Sage's R (3.4.4) rather than system's R (3.5.1, as packaged by Debian).
It turns out that I can't check out the branch related to this ticket (see sagedevel).
But manually fixing the typo in a local branch doesn fix the problem :
sage f rpy2 sage t long warnlong 47.5 src/sage/interfaces/r.pyDid you reinstall
rpy2
between those commands?
Nope. No reason to (see below).
I think
f
just removes the package and without reinstalling it before testing, god knows what happens.
Huh ?
sage advanced [ ... ] f  force build: install the packages even if they are already installed
Whadadjasmoke ?
Could you also post the result of
./sage R version # version of sage's R
charpent@zenbookflip:/usr/local/sage8$ ./sage R R version 3.4.4 (20180315)  "Someone to Lean On" Copyright (C) 2018 The R Foundation for Statistical Computing Platform: x86_64pclinuxgnu (64bit) R est un logiciel libre livré sans AUCUNE GARANTIE. Vous pouvez le redistribuer sous certaines conditions. Tapez 'license()' ou 'licence()' pour plus de détails. R est un projet collaboratif avec de nombreux contributeurs. Tapez 'contributors()' pour plus d'information et 'citation()' pour la façon de le citer dans les publications. Tapez 'demo()' pour des démonstrations, 'help()' pour l'aide en ligne ou 'help.start()' pour obtenir l'aide au format HTML. Tapez 'q()' pour quitter R.
Information already given, BTW...
R version # system R
charpent@zenbookflip:/usr/local/sage8$ R version R version 3.5.1 (20180702)  "Feather Spray" Copyright (C) 2018 The R Foundation for Statistical Computing Platform: x86_64pclinuxgnu (64bit) R is free software and comes with ABSOLUTELY NO WARRANTY. You are welcome to redistribute it under the terms of the GNU General Public License versions 2 or 3. For more information about these matters see http://www.gnu.org/licenses/.
Ditto...
./sage c 'import rpy2; print(rpy2.version)' # sages rpy version
charpent@zenbookflip:/usr/local/sage8$ ./sage c 'import rpy2; print(rpy2.__version__)' # sages rpy version 2.8.2
No surprise here...
./sage c 'from rpy2.rinterface import R_VERSION_BUILD; print(R_VERSION_BUILD)' # the R sages rpy was build with }}}
charpent@zenbookflip:/usr/local/sage8$ ./sage c 'from rpy2.rinterface import R_VERSION_BUILD; print(R_VERSION_BUILD)' ('3', '4.4', '', 74408L)
Ditto...
comment:18 in reply to: ↑ 17 ; followup: ↓ 20 Changed 3 years ago by
Replying to charpent:
I think
f
just removes the package and without reinstalling it before testing, god knows what happens.Huh ?
sage advanced [ ... ] f  force build: install the packages even if they are already installedWhadadjasmoke ?
That was just my assumption, I never checked to be honest.
Could you also post the result of
./sage R version # version of sage's R[...] Information already given, BTW...
[...] Ditto...
[...] Ditto...
Well some of that information wasn't entirely clear before and for others I didn't know where you got them. Now we're both on the same page.
Seems like I jumped to conclusions and misclassified the problem. The correct R seems to be used. Although I see no mention of RHOMES
anywhere in the rpy2 code while RHOME
is supposed to be the "R Installation Directory", so I don't know how it actually picked the correct one before.
What headline do you get when you execute help('c')
within sage R
? "Combine Values into a Vector or List" or "Combining Objects"?
comment:19 Changed 3 years ago by
 Summary changed from rpy2 uses system R instead of sages R to R help text differs on some systems
comment:20 in reply to: ↑ 18 Changed 3 years ago by
Replying to ghtimokau:
What headline do you get when you execute
help('c')
withinsage R
? "Combine Values into a Vector or List" or "Combining Objects"?
Combine Values into a Vector or List
comment:21 followup: ↓ 22 Changed 3 years ago by
Thats weird. The only reason I can think of is the one François already hinted to. Somehow there may be multiple help pages for c
.
What does from rpy2 import robjects; print([ o.title() for o in robjects.help.pages('c') ])
give when executed within sage?
comment:22 in reply to: ↑ 21 Changed 3 years ago by
Replying to ghtimokau:
Thats weird. The only reason I can think of is the one François already hinted to. Somehow there may be multiple help pages for
c
.What does
from rpy2 import robjects; print([ o.title() for o in robjects.help.pages('c') ])
give when executed within sage?
sage: from rpy2 import robjects; print([ o.title() for o in robjects.help.pages( ....: 'c') ]) ['Combining Objects', 'Combine Values into a Vector or List', 'NIMBLE language functions for Rlike vector construction', 'Transformation of coordinate systems', "Concatenating 'timeDate' Objects"]
comment:23 followup: ↓ 27 Changed 3 years ago by
Okay good, so we know what the problem is. There are multiple packages installed that provide a help page on the topic "c". My implementation in #26596 picks a different one by default than the R REPL does. The reason for that is that we cannot do plain eval('help("c")
), since for some reason rpy2 will then print the help page to stdout instead of returning it (as mentioned in the rpy2 docs). Instead we have to use rpy2s function robjects.help.pages("c")
, which returns a list of all help pages on the topic. I just picked the first one, assuming it would be the same one the R help()
function would display. But apparently that is not the case.
Are you really sure you haven't installed any additional R packages? For example one of the titles comes from the nimble
package, which is not installed by default. This is a bug either way, but if you really didn't install that package that makes it even weirder.
comment:24 Changed 3 years ago by
It looks like R actually also prefers nimbles help page, *if the package is loaded*. If it is in batch mode (otherwise it'll show a list of matching help pages and let the user select one), library(nimble); help(c)
will show nimble's help.
So I'll try to match the "only loaded packages" behaviour with rpy.
comment:25 Changed 3 years ago by
 Commit changed from aeb8d30f215a426b32fe096a5f22a3a25755b106 to 9a3276592ac7d0d1cc0de7208135eff2b899be02
comment:27 in reply to: ↑ 23 Changed 3 years ago by
Replying to ghtimokau:
Are you really sure you haven't installed any additional R packages?
Sorry : I have probably misunderstood you the first time you've asked the question : I didn't add any new package to my Sage R installation. However, some packages are present :
dim(installed.packages()) [1] 494 16
comment:28 Changed 3 years ago by
Thats one less mystery. Good thing you did too, otherwise we wouldn't have found this bug :)
Does my latest commit fix it for you?
comment:29 Changed 3 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_work
Same result. Sorry...
In system's R, asking help on an overloaded system leads to a presentation of the alternatives and a question asking which symbol's help is needed. There is (rightly, IMHO) no ettempot to guess which one is the "right" one... :
> library(nimble) nimble version 0.612 is loaded. For more information on NIMBLE and a User Manual, please visit http://Rnimble.org. Attachement du package : ‘nimble’ The following object is masked from ‘package:stats’: simulate > ?c Help on topic ‘c’ was found in the following packages: Package Library nimble /usr/local/lib/R/sitelibrary base /usr/lib/R/library
comment:30 Changed 3 years ago by
That is surprising, I could reproduce the issue when installing nible
and the latest commit fixed it for me. What does ./sage c "from rpy2.robjects import r; print(r('loadedNamespaces()'))"
print?
R will usually let you select, but in "batch mode" it will just give you the first. I agree that that is not ideal behaviour, but I matched the behaviour of the old r
interface (which parsed the batch output) to preserve backwards compatibility.
comment:31 followup: ↓ 32 Changed 3 years ago by
 Cc embray fbissey jdemeyer added
I begin to think that #26596 was a mistake.
comment:32 in reply to: ↑ 31 ; followup: ↓ 33 Changed 3 years ago by
Replying to charpent:
I begin to think that #26596 was a mistake.
To qualify : R is a complex beast ; most notably, it offers some "interactive" abilities (e. g. finding a data point from a plot by clicking on it), which I don't use regularly, but might be used by some Sage users.
Similarly, I know that (some) packages among the 13473 packages available as of this morning do engage in interaction with the users through widgets (e. g. Tcl/Tk?), menus, dialog boxes, etc...
As far as I can tell, using rpy2
(which entails "batch mode") loses these abilities. Therefore, Sage's R can't replace system's R for these uses.
I haven't the foggiest idea of the importance of these abilities for Sage's R users. Similarly, testing the 13474 available packages for usability with Sage's R seems undoable.
The only way out I can see presently is to
 remove Sage's R,
 depend on system's R,
 disclaim usability of "interactive" R functions from Sage,
which furthermore opens the nightmare of maintaining compatibility across versions.
ISTR that Erik Bray "had ideas" about how to do that.
Thoughts ?
comment:33 in reply to: ↑ 32 Changed 3 years ago by
Replying to charpent:
I begin to think that #26596 was a mistake.
I very much disagree.
Replying to charpent:
Replying to charpent:
I begin to think that #26596 was a mistake.
To qualify : R is a complex beast ; most notably, it offers some "interactive" abilities (e. g. finding a data point from a plot by clicking on it), which I don't use regularly, but might be used by some Sage users.
Do you have any concrete example of some feature that worked before but doesn't work now?
Similarly, I know that (some) packages among the 13473 packages available as of this morning do engage in interaction with the users through widgets (e. g. Tcl/Tk?), menus, dialog boxes, etc...
As far as I can tell, using
rpy2
(which entails "batch mode") loses these abilities. Therefore, Sage's R can't replace system's R for these uses.
It was already "batch mode" before. Now it uses the C interface that R provides instead of parsing the REPL output, which is much more robust.
I haven't the foggiest idea of the importance of these abilities for Sage's R users. Similarly, testing the 13474 available packages for usability with Sage's R seems undoable.
The only way out I can see presently is to
 remove Sage's R,
 depend on system's R,
I don't see how any of these options would improve anything.
 disclaim usability of "interactive" R functions from Sage,
which furthermore opens the nightmare of maintaining compatibility across versions.
That nightmare is *exactly* the point of #26596.
ISTR that Erik Bray "had ideas" about how to do that.
Thoughts ?
comment:35 followups: ↓ 36 ↓ 37 Changed 3 years ago by
In my opinion any known issue that may cause test failures for some users should be a blocker. Test failures in a released sage version should always indicate that there is a real, not already reported issue.
This is very low severity, but if we can't solve it before release (waiting on @charpent's reply to my last post) I suggest that we at least mark the test as a known failure.
comment:36 in reply to: ↑ 35 Changed 3 years ago by
Replying to ghtimokau:
In my opinion any known issue that may cause test failures for some users should be a blocker. Test failures in a released sage version should always indicate that there is a real, not already reported issue.
This is very low severity, but if we can't solve it before release (waiting on @charpent's reply to my last post) I suggest that we at least mark the test as a known failure.
I think, if nothing else, the tests could be marked as known failures on the relevant system using #26784, which still needs review (annoyingly, considering that when I first raised the suggestion as a solution to a different problem Volker gave me "two days" to implement it as though that problem were high priority; I did it in one day and yet two weeks later...)
comment:37 in reply to: ↑ 35 ; followups: ↓ 38 ↓ 39 Changed 3 years ago by
Replying to ghtimokau:
[ Snip... ]
This is very low severity, but if we can't solve it before release (waiting on @charpent's reply to my last post) I suggest that we at least mark the test as a known failure.
Sorry : this one slipped from my queue.
What worries me is that we introduce two distinct behaviours (using the interpreter and using the interactive interface to the library) without documenting the difference (which used to be irrelevant).
For example, in the "help" failures, we have a problem because the old expect interface happened to select whatever was available in an "empty" interpreter (implicitly launched by the interface), whereas our library call picks (the first available answer in installed packages (installation dependent).
We should :
 document this behaviour
 specify how to select the package containing the function we wissh help about
 use this mechanism to specify explicitly the relevant library argument ("base", in our case) and test agaist this specific help text.
Furthermore, we should be aware of the possibility of such surprises. I do not see a way to search systematically for them. But I still think we should document te possibility of such surprises.
comment:38 in reply to: ↑ 37 Changed 3 years ago by
Replying to embray:
Replying to ghtimokau:
In my opinion any known issue that may cause test failures for some users should be a blocker. Test failures in a released sage version should always indicate that there is a real, not already reported issue.
This is very low severity, but if we can't solve it before release (waiting on @charpent's reply to my last post) I suggest that we at least mark the test as a known failure.
I think, if nothing else, the tests could be marked as known failures on the relevant system using #26784, which still needs review (annoyingly, considering that when I first raised the suggestion as a solution to a different problem Volker gave me "two days" to implement it as though that problem were high priority; I did it in one day and yet two weeks later...)
Why not the regular # known failure
marker? Marking the whole file seems overkill.
comment:39 in reply to: ↑ 37 Changed 3 years ago by
Replying to charpent:
What worries me is that we introduce two distinct behaviours (using the interpreter and using the interactive interface to the library) without documenting the difference (which used to be irrelevant).
The new R interface also effectively runs the equivalent to an interpreter internally.
For example, in the "help" failures, we have a problem because the old expect interface happened to select whatever was available in an "empty" interpreter (implicitly launched by the interface), whereas our library call picks (the first available answer in installed packages (installation dependent).
In fact I just tested the behaviour under the old interface. r.library('nimble'); r.help('c')
hangs indefinitely. The old R interface was seriously broken. Which is why I was asking for a concrete regression (besides this help pages issue).
This issue in particular is due to R being too helpful and automatically piping the help
output into a pager, even when called through the library. Because of that, rpy2 had to implement its own custom help implementation.
I put a lot of effort into compatibility with the old interface (where it was not broken). So if there is a difference, that is a bug.
comment:40 Changed 3 years ago by
 Commit changed from 9a3276592ac7d0d1cc0de7208135eff2b899be02 to bd0f8f2b77fd2853d63890f6d5b5aa6479c8caef
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bd0f8f2  Only consider loaded packages for help pages in R

comment:41 Changed 3 years ago by
 Status changed from needs_work to needs_review
Okay this should really be fixed now. I didn't actually use the new help function I wrote before, which is why it didn't work.
This time I made sure I reproduced the error before testing the fix. Sicne this fixes a regression, I think this should be included in the next rc.
comment:42 Changed 3 years ago by
Not entirely sure if the original typo fix also belongs here. I have no idea why it worked before, but apparently it doesn't really matter weather its RHOME
or RHOMES
. Can't really hurt to fix it anyways though.
comment:43 Changed 3 years ago by
 Status changed from needs_review to positive_review
Applied on top of 8.6.beta1, builds and passes ptestlong without any problem ==> positive_review
.
comment:44 Changed 3 years ago by
 Status changed from positive_review to needs_work
Reviewer name...
comment:45 Changed 3 years ago by
 Reviewers set to Emmanuel Charpentier
 Status changed from needs_work to positive_review
Wups...
comment:46 Changed 3 years ago by
 Branch changed from u/ghtimokau/rpy2buildtypo to bd0f8f2b77fd2853d63890f6d5b5aa6479c8caef
 Resolution set to fixed
 Status changed from positive_review to closed
I've looked at rpy2s build script. It contains the line
That looks like a typo. rpy2 respects the environment variable
RHOME
, without thes
. Probably didn't cause any issues before becauserpy2
wasn't used very heavily before #26596.