Opened 3 years ago

Closed 3 years ago

#26791 closed defect (fixed)

R help text differs on some systems

Reported by: gh-timokau Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description (last modified by charpent)

See https://groups.google.com/d/msg/sage-release/MX-SnQJh8EE/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 gh-timokau

I've looked at rpy2s build script. It contains the line

export RHOMES="$SAGE_LOCAL"/lib/R

That looks like a typo. rpy2 respects the environment variable RHOME, without the s. Probably didn't cause any issues before because rpy2 wasn't used very heavily before #26596.

comment:2 Changed 3 years ago by gh-timokau

  • Authors set to Timo Kaufmann
  • Branch set to u/gh-timokau/rpy2-build-typo
  • Commit set to c25df8dc460e3d810ea47f0c2340f6b3252ff746
  • Status changed from new to needs_review

New commits:

c25df8dFix rpy2 build script typo

comment:3 follow-up: Changed 3 years ago by gh-timokau

  • Cc charpent added

@charpent please test.

comment:4 Changed 3 years ago by fbissey

That's a case where you want a revision bump in the package version. We want everyone to re-install this.

comment:5 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by charpent

Replying to gh-timokau:

@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 git

  • Commit changed from c25df8dc460e3d810ea47f0c2340f6b3252ff746 to 2af44d2cf377bf9509d9d5a4d9f044c99ba55759

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2af44d2Fix rpy2 build script typo

comment:7 follow-up: Changed 3 years ago by gh-timokau

Right, added the package bump.

How do you know which version rpy2 actually used?


New commits:

2af44d2Fix rpy2 build script typo

comment:8 Changed 3 years ago by git

  • Commit changed from 2af44d2cf377bf9509d9d5a4d9f044c99ba55759 to aeb8d30f215a426b32fe096a5f22a3a25755b106

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

aeb8d30Fix rpy2 build script typo

comment:9 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by fbissey

Replying to gh-timokau:

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 ; follow-up: Changed 3 years ago by charpent

Replying to charpent:

Replying to gh-timokau:

@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 sage-devel).

But manually fixing the typo in a local branch doesn fix the problem :

sage -f rpy2
sage -t --long --warn-long 47.5 src/sage/interfaces/r.py 

gives the same result :

----------------------------------------------------------------------
sage -t --long --warn-long 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:

aeb8d30Fix rpy2 build script typo

comment:11 follow-up: Changed 3 years ago by charpent

  • Status changed from needs_review to needs_work

Not solved (see above)...

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by 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?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by charpent

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/package-version.txt get bumped ?

comment:14 in reply to: ↑ 13 Changed 3 years ago by fbissey

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/package-version.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 gh-timokau

Replying to fbissey:

Replying to gh-timokau:

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 ; follow-up: Changed 3 years ago by gh-timokau

Replying to charpent:

Replying to charpent:

Replying to gh-timokau:

@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 sage-devel).

But manually fixing the typo in a local branch doesn fix the problem :

sage -f rpy2
sage -t --long --warn-long 47.5 src/sage/interfaces/r.py 

Did you re-install 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 ; follow-up: Changed 3 years ago by charpent

Replying to gh-timokau:

Replying to charpent:

Replying to charpent:

Replying to gh-timokau:

@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 sage-devel).

But manually fixing the typo in a local branch doesn fix the problem :

sage -f rpy2
sage -t --long --warn-long 47.5 src/sage/interfaces/r.py 

Did you re-install 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@zen-book-flip:/usr/local/sage-8$ ./sage -R

R version 3.4.4 (2018-03-15) -- "Someone to Lean On"
Copyright (C) 2018 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

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@zen-book-flip:/usr/local/sage-8$ R --version
R version 3.5.1 (2018-07-02) -- "Feather Spray"
Copyright (C) 2018 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

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@zen-book-flip:/usr/local/sage-8$ ./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@zen-book-flip:/usr/local/sage-8$ ./sage -c 'from rpy2.rinterface import R_VERSION_BUILD; print(R_VERSION_BUILD)'
('3', '4.4', '', 74408L)

Ditto...

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by gh-timokau

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 installed

Whadadjasmoke ?

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 gh-timokau

  • 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 charpent

Replying to gh-timokau:

What headline do you get when you execute help('c') within sage -R? "Combine Values into a Vector or List" or "Combining Objects"?

Combine Values into a Vector or List

comment:21 follow-up: Changed 3 years ago by gh-timokau

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 charpent

Replying to gh-timokau:

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 R-like vector construction', 'Transformation of coordinate systems', "Concatenating 'timeDate' Objects"]

comment:23 follow-up: Changed 3 years ago by gh-timokau

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 gh-timokau

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 git

  • Commit changed from aeb8d30f215a426b32fe096a5f22a3a25755b106 to 9a3276592ac7d0d1cc0de7208135eff2b899be02

Branch pushed to git repo; I updated commit sha1. New commits:

e035e77Merge remote-tracking branch 'origin/develop' into u/gh-timokau/rpy2-build-typo
9a32765Only consider loaded packages for help pages in R

comment:26 Changed 3 years ago by gh-timokau

  • Status changed from needs_work to needs_review

Please test.

comment:27 in reply to: ↑ 23 Changed 3 years ago by charpent

Replying to gh-timokau:

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 gh-timokau

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 charpent

  • 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.6-12 is loaded.
For more information on NIMBLE and a User Manual,
please visit http://R-nimble.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/site-library
  base                  /usr/lib/R/library
Last edited 3 years ago by charpent (previous) (diff)

comment:30 Changed 3 years ago by gh-timokau

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 follow-up: Changed 3 years ago by charpent

  • Cc embray fbissey jdemeyer added

I begin to think that #26596 was a mistake.

comment:32 in reply to: ↑ 31 ; follow-up: Changed 3 years ago by 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.

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 gh-timokau

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 ?

Last edited 3 years ago by gh-timokau (previous) (diff)

comment:34 Changed 3 years ago by vbraun

  • Priority changed from blocker to major

Not a blocker..

comment:35 follow-ups: Changed 3 years ago by gh-timokau

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 embray

Replying to gh-timokau:

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...)

Last edited 3 years ago by embray (previous) (diff)

comment:37 in reply to: ↑ 35 ; follow-ups: Changed 3 years ago by charpent

Replying to gh-timokau:

[ 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 gh-timokau

Replying to embray:

Replying to gh-timokau:

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 gh-timokau

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 git

  • Commit changed from 9a3276592ac7d0d1cc0de7208135eff2b899be02 to bd0f8f2b77fd2853d63890f6d5b5aa6479c8caef

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bd0f8f2Only consider loaded packages for help pages in R

comment:41 Changed 3 years ago by gh-timokau

  • 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 gh-timokau

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 charpent

  • 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 vbraun

  • Status changed from positive_review to needs_work

Reviewer name...

comment:45 Changed 3 years ago by charpent

  • Reviewers set to Emmanuel Charpentier
  • Status changed from needs_work to positive_review

Wups...

comment:46 Changed 3 years ago by vbraun

  • Branch changed from u/gh-timokau/rpy2-build-typo to bd0f8f2b77fd2853d63890f6d5b5aa6479c8caef
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.