Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#29441 closed enhancement (fixed)

upgrade rpy2 package 2.8.2 -> 3.3.5, upgrade R to 3.6.3, add new dependencies

Reported by: Vincent Delecroix Owned by:
Priority: major Milestone: sage-9.2
Component: packages: standard Keywords:
Cc: Matthias Köppe, Emmanuel Charpentier, Timo Kaufmann, Isuru Fernando, Samuel Lelièvre, Antonio Rojas, Erik Bray, Dima Pasechnik Merged in:
Authors: Vincent Delecroix, Matthias Koeppe Reviewers: Emmanuel Charpentier, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 54d0f99 (Commits, GitHub, GitLab) Commit:
Dependencies: #29851, #30064, #30118, #30067, #30147, #29929, #30149 Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

Main changes:

  • add small patches to setup.py that
    • disables printing on stdout (that perturbs pip) - accepted upstream
    • removes the build dependency on pytest
  • some more dependencies (pytest, numpy, tzlocal) conditional on SAGE_CHECK!=no

tarballs: see checksums.ini (to download automatically, use ./configure --enable-download-from-upstream-url)

Upstream issues and PR

Attachments (2)

rdoctest.txt.gz (11.9 KB) - added by Emmanuel Charpentier 2 years ago.
Doctest for src/sage/interfaces/r.py
chkerrs.txt (105.8 KB) - added by Emmanuel Charpentier 2 years ago.

Download all attachments as: .zip

Change History (122)

comment:1 Changed 3 years ago by Vincent Delecroix

Cc: Emmanuel Charpentier added
Description: modified (diff)

The cygwin.patch does not longer apply.

comment:2 Changed 3 years ago by Vincent Delecroix

Description: modified (diff)

comment:3 Changed 3 years ago by Vincent Delecroix

Description: modified (diff)

comment:4 Changed 3 years ago by Vincent Delecroix

pytest has a lot of dependencies

  • attrs
  • importlib-metadata
  • more-itertools
  • pluggy
  • py
  • zipp

The easiest might be to patch rpy2 to not depend on pytest. I suspect that only the test suite of rpy2 depends on pytest.

comment:5 Changed 3 years ago by Vincent Delecroix

Description: modified (diff)

comment:6 Changed 3 years ago by Vincent Delecroix

Good news: tests pass on my machine!

Last edited 3 years ago by Vincent Delecroix (previous) (diff)

comment:7 Changed 3 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.2

comment:8 Changed 3 years ago by Matthias Köppe

See #28998 for pytest

comment:9 in reply to:  4 ; Changed 3 years ago by Emmanuel Charpentier

Description: modified (diff)
Milestone: sage-9.2sage-9.1

Replying to vdelecroix:

pytest has a lot of dependencies

  • attrs
  • importlib-metadata
  • more-itertools
  • pluggy
  • py
  • zipp

Tall order, indeed...

The easiest might be to patch rpy2 to not depend on pytest. I suspect that only the test suite of rpy2 depends on pytest.

My first reaction was: No, no, no, no, no. And no.". A patched version has a very heavy price in terms of maintenance each and every update entails patch checking/upgrading, up to the point the whole mess becomes unmaintainable.

The maintenance price of the added dependencies should be considered, of course, but I suspect that it may be lower than the price of a patch to rpy2.

comment:10 in reply to:  8 ; Changed 3 years ago by Vincent Delecroix

Replying to mkoeppe:

See #28998 for pytest

Thanks for the pointer. Would pytest be installed as a "standard" package?

comment:11 in reply to:  8 Changed 3 years ago by Emmanuel Charpentier

Replying to mkoeppe:

See #28998 for pytest

Dors it need much work ?

comment:12 in reply to:  10 Changed 3 years ago by Emmanuel Charpentier

Replying to vdelecroix:

Replying to mkoeppe:

See #28998 for pytest

Thanks for the pointer. Would pytest be installed as a "standard" package?

It would have to : r depends on rpy2, which would depend on pytest. ISTR that you yourself stated that having a standard package depend on an optional package would be madness (and I agree...). Therefore...

comment:13 Changed 3 years ago by Vincent Delecroix

Branch: public/29441
Commit: c525fc2441b5a93f7c38e3c9bdff170b0454ef3b

New commits:

de753f8upgrade rpy2 version
7ca1336remove cygwin.patch (rpy2)
c525fc2add setup.patch (rpy2)

comment:14 Changed 3 years ago by Vincent Delecroix

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

comment:15 in reply to:  9 ; Changed 3 years ago by Matthias Köppe

Replying to charpent:

Milestone changed from sage-9.2 to sage-9.1

Was this intentional? We definitely cannot drop py2 support for 9.1

comment:16 in reply to:  15 Changed 3 years ago by Vincent Delecroix

Milestone: sage-9.1sage-9.2

Replying to mkoeppe:

Replying to charpent:

Milestone changed from sage-9.2 to sage-9.1

Was this intentional? We definitely cannot drop py2 support for 9.1

Agreed. This was a mistake.

comment:17 Changed 3 years ago by Vincent Delecroix

Description: modified (diff)

comment:18 in reply to:  14 ; Changed 3 years ago by Emmanuel Charpentier

Replying to vdelecroix:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

Replying to vdelecroix:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

1) Can rpy2 use packages hypothetically installed by an hypothetical package implementing #28998 ?

2) should we instead wait for an upstream-fixed rpy2 (supposing that upstream agrees with you...) ?

comment:19 in reply to:  18 Changed 3 years ago by Vincent Delecroix

Replying to charpent:

Replying to vdelecroix:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

2) should we instead wait for an upstream-fixed rpy2 (supposing that upstream agrees with you...) ?

YES

comment:20 Changed 3 years ago by Matthias Köppe

Description: modified (diff)

rpy2 should be patched so that pytest is not an actual dependency, but test-only.

rpy2 is missing modern metadata such as requirements.txt, test-requirements.txt, tox.ini where things like this are commonly declared.

#28998 would install pytest only when SAGE_CHECK=yes.

comment:21 Changed 3 years ago by Matthias Köppe

By the way, I recommend that you add the new upstream_url fields to checksum.ini. Makes it easier to test the ticket.

comment:22 Changed 3 years ago by Matthias Köppe

(See #29425 for an example)

comment:23 in reply to:  20 ; Changed 3 years ago by Vincent Delecroix

Replying to mkoeppe:

Description modified (diff)

@mkoeppe: Did you intentionally erase my modifications to the ticket description!?

comment:24 Changed 3 years ago by git

Commit: c525fc2441b5a93f7c38e3c9bdff170b0454ef3be672b6eef2b52c368c8288e33bdef57391be36b0

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

e672b6eadd upstream_url field to rpy2 checksums.ini

comment:25 Changed 3 years ago by Vincent Delecroix

Description: modified (diff)

comment:26 Changed 3 years ago by git

Commit: e672b6eef2b52c368c8288e33bdef57391be36b086d0d175c7e8d840ff6685910825f026b537f012

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

86d0d17let rpy2 not depend on pytest

comment:27 in reply to:  23 Changed 3 years ago by Matthias Köppe

Replying to vdelecroix:

Replying to mkoeppe:

Description modified (diff)

@mkoeppe: Did you intentionally erase my modifications to the ticket description!?

No, sorry! Looks like on this ticket trac is playing funny tricks

comment:28 Changed 3 years ago by git

Commit: 86d0d175c7e8d840ff6685910825f026b537f0125eee83e9693a6e146b1a135329b8e6c153fe1288

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

a1debabcffi package (rpy2 dependency)
a8b6e66pycparser (rpy2 dependency)
5eee83eupdate rpy2 dependencies

comment:29 Changed 3 years ago by Vincent Delecroix

Status: newneeds_review

Ready for tests (especially on cygwin), but not for merging!

comment:30 Changed 3 years ago by Matthias Köppe

If you want to test cygwin via github actions, be sure to use #29403

comment:31 Changed 3 years ago by Timo Kaufmann

Cc: Timo Kaufmann added

comment:32 Changed 3 years ago by Vincent Delecroix

upstream is not (totally) convinced by dropping the pytest dependency. I propose to however keep the patch in Sage (on archlinux rpy2 does not depend on pytest either).

I will add a commit that still installs the test as part of rpy2 (it did not make much sense to remove it).

comment:33 Changed 3 years ago by git

Commit: 5eee83e9693a6e146b1a135329b8e6c153fe1288308d39c4ce3e477716aa4408def52c68c1416482

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

308d39cstill install tests along rpy2

comment:34 Changed 3 years ago by Vincent Delecroix

Authors: Vincent Delecroix
Description: modified (diff)

comment:35 Changed 3 years ago by Matthias Köppe

Status: needs_reviewneeds_work

Needs rebasing

comment:36 Changed 2 years ago by Matthias Köppe

#29813 adds pytest

comment:37 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:38 Changed 2 years ago by Matthias Köppe

Latest rpy2 is now 3.3.3

comment:39 Changed 2 years ago by git

Commit: 308d39c4ce3e477716aa4408def52c68c141648281f580eff5007a666dc712e5219d47f5933e1573

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

68ea7beupgrade rpy2 version
f1cd4e2remove cygwin.patch (rpy2)
68f6f92add setup.patch (rpy2)
b550f43add upstream_url field to rpy2 checksums.ini
2cd2191let rpy2 not depend on pytest
b76ee25cffi package (rpy2 dependency)
84ded30pycparser (rpy2 dependency)
2e0fe73update rpy2 dependencies
81f580estill install tests along rpy2

comment:40 Changed 2 years ago by Matthias Köppe

Rebased on 9.2.beta2

comment:41 Changed 2 years ago by Matthias Köppe

Description: modified (diff)
Summary: upgrade rpy2 package 2.8.2 -> 3.2.7upgrade rpy2 package 2.8.2 -> 3.3.4, add new dependencies

comment:42 Changed 2 years ago by Matthias Köppe

Cc: Isuru Fernando Samuel Lelièvre Antonio Rojas added

comment:43 Changed 2 years ago by git

Commit: 81f580eff5007a666dc712e5219d47f5933e1573d4c16728171baa763dcb653f83aa485fdbeedefa

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

d4c1672build/pkgs: Upgrade rpy2 to 3.3.4, use patterns for upstream_urls of dependencies

comment:44 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

Patches need updating!

comment:46 Changed 2 years ago by Matthias Köppe

Authors: Vincent DelecroixVincent Delecroix, Matthias Koeppe
Dependencies: #29813

comment:47 Changed 2 years ago by git

Commit: d4c16728171baa763dcb653f83aa485fdbeedefad33797fe7e8f7a069a39eb446419592916fc90a9

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

28552a2Update patches
04b4c67build/pkgs/pytest: New
55babfasrc/bin/sage [-i]: Set SAGE_CHECK here so that Makefile dependencies can depend on it
bedc7aebuild/make/Makefile.in: Allow pip packages as dependencies
e0db954Merge branch 't/29813/add_pytest_as_a_type_optional__source_pip_package' into t/29441/public/29441
20133adbuild/pkgs/rpy2: Add spkg-check.in, add conditional dep on pytest, simplify spkg-install.in
c02362ebuild/pkgs/tzlocal: New (pip package)
d33797fbuild/pkgs/rpy2/dependencies: Add conditional dep on tzlocal, numpy

comment:48 in reply to:  18 Changed 2 years ago by Matthias Köppe

Replying to charpent:

Replying to vdelecroix:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

1) Can rpy2 use packages hypothetically installed by an hypothetical package implementing #28998 ?

Yes, this is done here on the ticket now, using #29813 instead of the larger #28998.

comment:49 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

comment:50 Changed 2 years ago by Matthias Köppe

comment:51 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:52 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:53 Changed 2 years ago by git

Commit: d33797fe7e8f7a069a39eb446419592916fc90a9b66e9df749993b975920a440f870bc3f6df19884

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

b66e9dfbuild/pkgs/{cffi,pycparser}/dependencies: New

comment:54 Changed 2 years ago by Matthias Köppe

Dependencies: #29813

comment:55 Changed 2 years ago by git

Commit: b66e9df749993b975920a440f870bc3f6df198844c7f18dfc607ebe77bce93ea1db14509fc7a5c10

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

539c182build/make/install: Do not depend on src/bin/sage-version.sh
761092cMerge branch 't/29987/build_make_install__do_not_depend_on_src_bin_sage_version_sh' into t/30064/fix_tox_docker_builds_broken_by__29884
f2efa6asrc/doc/bootstrap: Create the directory src/doc/en/reference/repl if it does not exist
b7bf43bbuild/bin/write-dockerfile.sh: ADD src/bin for bootstrapping, needed by src/doc/bootstrap after #29884
365ce61Merge branch 'u/mkoeppe/fix_tox_docker_builds_broken_by__29884' of git://trac.sagemath.org/sage into HEAD
1e7becctox.ini [debian-buster, -sid]: IGNORE_MISSING_SYSTEM_PACKAGES=yes because of libpython3.7-dev
fb61a31Merge branch 'u/mkoeppe/tox_ini__debian_bullseye___sid_have_python3_8_instead_of_3_7' of git://trac.sagemath.org/sage into 9.2.beta3+ci-fixes
4c7f18dMerge branch '9.2.beta3+ci-fixes' into t/29441/public/29441

comment:56 Changed 2 years ago by Matthias Köppe

Dependencies: #29851, #30064

comment:57 in reply to:  50 Changed 2 years ago by Matthias Köppe

Replying to mkoeppe:

Build testing runs at https://github.com/mkoeppe/sage/actions/runs/157245915

Some unrelated problems are in the way. For example, on ubuntu-eoan-minimal,

  [pytest]     Retrying (Retry(total=0, connect=None, read=None, redirect=None, status=None)) after connection broken by 'SSLError("Can't connect to HTTPS URL because the SSL module is not available.")': /simple/pytest/
  [pytest]     Could not fetch URL https://pypi.org/simple/pytest/: There was a problem confirming the ssl certificate: HTTPSConnectionPool(host='pypi.org', port=443): Max 

comment:58 Changed 2 years ago by Matthias Köppe

Dependencies: #29851, #30064#29851, #30064, #30118

comment:59 Changed 2 years ago by git

Commit: 4c7f18dfc607ebe77bce93ea1db14509fc7a5c103b14bf51517e2c8c77a49dcc286fe6dfaf38dc48

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

25393b0Handle SAGE_CHECK_PACKAGES in build/make/Makefile.in, not sage-spkg
833ff0eMerge branch 't/30118/handle_sage_check_packages_in_build_make_makefile_in__not_sage_spkg' into t/29441/public/29441
3b14bf5build/pkgs/rpy2/dependencies: Conditionalize on SAGE_CHECK_rpy2

comment:60 Changed 2 years ago by Matthias Köppe

Cc: Erik Bray Dima Pasechnik added

comment:61 Changed 2 years ago by Emmanuel Charpentier

Status: needs_reviewneeds_work
  • On Debian testing running n core i7 + 16 GB RAM
  • Sage configured to use systemwide R
  • Rebase on develop at 9.2.beta5
sage: r.sessionInfo()
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-24-69f706c2bf29> in <module>()
----> 1 r.sessionInfo()

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/r.py in __call__(self, *args, **kwds)
   2001             [1] 3
   2002         """
-> 2003         return self._parent.function_call(self._name, args=list(args), kwds=kwds)
   2004 
   2005 def is_RElement(x):

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/r.py in function_call(self, function, args, kwds)
   1078         self._check_valid_function_name(function)
   1079         return self.new("%s(%s)"%(function, ",".join([s.name() for s in args] +
-> 1080                                                      [self._sage_to_r_name(key)+'='+kwds[key].name() for key in kwds ] )))
   1081 
   1082     def call(self, function_name, *args, **kwds):

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/interface.py in new(self, code)
    368 
    369     def new(self, code):
--> 370         return self(code)
    371 
    372     ###################################################################

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/interface.py in __call__(self, x, name)
    294 
    295         if isinstance(x, str):
--> 296             return cls(self, x, name=name)
    297         try:
    298             # Special methods do not and should not have an option to

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/interface.py in __init__(self, parent, value, is_name, name)
    716         else:
    717             try:
--> 718                 self._name = parent._create(value, name=name)
    719             except (TypeError, RuntimeError, ValueError) as x:
    720                 raise TypeError(x)

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/interface.py in _create(self, value, name)
    499     def _create(self, value, name=None):
    500         name = self._next_var_name() if name is None else name
--> 501         self.set(name, value)
    502         return name
    503 

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/r.py in set(self, var, value)
   1124         """
   1125         cmd = '%s <- %s'%(var,value)
-> 1126         out = self.eval(cmd)
   1127 
   1128     def get(self, var):

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/interfaces/r.py in eval(self, code, *args, **kwds)
   1337         """
   1338         self._lazy_init()
-> 1339         return str(robjects.r(code)).rstrip()
   1340 
   1341 

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__getattr__ (build/cythonized/sage/misc/lazy_import.c:3525)()
    319             True
    320         """
--> 321         return getattr(self.get_object(), attr)
    322 
    323     # We need to wrap all the slot methods, as they are not forwarded

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.get_object (build/cythonized/sage/misc/lazy_import.c:2336)()
    186         if likely(self._object is not None):
    187             return self._object
--> 188         return self._get_object()
    189 
    190     cpdef _get_object(self):

/usr/local/sage-9/local/lib/python3.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport._get_object (build/cythonized/sage/misc/lazy_import.c:2575)()
    218         elif self._at_startup and not startup_guard:
    219             print('Option ``at_startup=True`` for lazy import {0} not needed anymore'.format(self._name))
--> 220         self._object = getattr(__import__(self._module, {}, {}, [self._name]), self._name)
    221         name = self._as_name
    222         if self._deprecation is not None:

/usr/local/sage-9/local/lib/python3.7/site-packages/rpy2/robjects/__init__.py in <module>()
     24 
     25 from . import conversion
---> 26 from . import vectors
     27 from . import language
     28 

/usr/local/sage-9/local/lib/python3.7/site-packages/rpy2/robjects/vectors.py in <module>()
     14 import time
     15 import pytz
---> 16 import tzlocal
     17 from datetime import date, datetime, timedelta, timezone
     18 from time import struct_time, mktime, tzname

ModuleNotFoundError: No module named 'tzlocal'

All othetr attemps end up similarly.

==>needs_work

Changed 2 years ago by Emmanuel Charpentier

Attachment: rdoctest.txt.gz added

Doctest for src/sage/interfaces/r.py

comment:62 in reply to:  61 Changed 2 years ago by Emmanuel Charpentier

Replying to charpent:

  • On Debian testing running n core i7 + 16 GB RAM
  • Sage configured to use systemwide R
  • Rebase on develop at 9.2.beta5

[ Snip... ]

BTW, sage -t --long src/sage/interfaces/r.py gives the enclosed rdoctest.txt.gz

HTH,

comment:63 Changed 2 years ago by Dima Pasechnik

yeah, I see this error too.

comment:64 Changed 2 years ago by Dima Pasechnik

missing dep, can be installed using pip:

$ ./sage --pip install tzlocal
Collecting tzlocal
  Downloading tzlocal-2.1-py2.py3-none-any.whl (16 kB)
Requirement already satisfied: pytz in ./local/lib/python3.7/site-packages (from tzlocal) (2018.7)
Installing collected packages: tzlocal
Successfully installed tzlocal-2.1
dimpase@penguin:~/sage$ ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.2.beta5, Release Date: 2020-07-12               │
│ Using Python 3.7.3. Type "help()" for help.                        │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: r.sessionInfo()
R version 3.5.2 (2018-12-20)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Debian GNU/Linux 10 (buster)

Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/openblas/libblas.so.3
LAPACK: /usr/lib/x86_64-linux-gnu/openblas/liblapack.so.3

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] tools     stats     graphics  grDevices utils     datasets  methods  
[8] base     

loaded via a namespace (and not attached):
[1] compiler_3.5.2

comment:65 in reply to:  64 Changed 2 years ago by Emmanuel Charpentier

Replying to dimpase:

missing dep, can be installed using pip:

[ Snip... ]

Could you add the relevant commit(s) to this branch before a new test ?

comment:66 Changed 2 years ago by Dima Pasechnik

this seems to need addition of a new pip package, no?

comment:67 Changed 2 years ago by Dima Pasechnik

oops, the package is there, it's just not getting installed for some reason, I guess due to something fishy in

  • build/pkgs/rpy2/dependencies

    a b  
    1 $(PYTHON) r six | $(PYTHON_TOOLCHAIN)
     1$(PYTHON) r cffi | $(PYTHON_TOOLCHAIN) pycparser $(and $(filter-out no,$(SAGE_CHECK_rpy2)), pytest tzlocal numpy)

comment:68 Changed 2 years ago by Matthias Köppe

tzlocal is only pulled in for tests (SAGE_CHECK=yes). Is it needed for normal installation or at runtime as well?

comment:69 Changed 2 years ago by Matthias Köppe

In that case, we would have to add it as a normal package.

comment:70 Changed 2 years ago by Matthias Köppe

... and update package pytz to 2020.1

comment:71 Changed 2 years ago by Matthias Köppe

Looking at the traceback in comment 61, the answer seems to be "yes".

I'll prepare the pytz update first.

comment:72 Changed 2 years ago by git

Commit: 3b14bf51517e2c8c77a49dcc286fe6dfaf38dc485f93e7f059fcbfa3eecf1dd22112fb647498a506

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

5f93e7fbuild/pkgs/pytz: Update to 2020.1

comment:73 Changed 2 years ago by git

Commit: 5f93e7f059fcbfa3eecf1dd22112fb647498a506091ed905e020537a18ff6f7c1251c87253537a0d

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

091ed90build/pkgs/tzlocal: Make it a normal, standard package (new dep of standard package rpy2)

comment:74 Changed 2 years ago by git

Commit: 091ed905e020537a18ff6f7c1251c87253537a0db01e92dd86b042c17d2bb17d44354d60bef861ab

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

b01e92dbuild/pkgs/rpy2/dependencies: Add tzlocal pytz as normal dependencies

comment:75 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

comment:76 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:77 Changed 2 years ago by git

Commit: b01e92dd86b042c17d2bb17d44354d60bef861ab4ce897074e0570143ec4642a91bf2d846f313635

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

4ce8970build/pkgs/rpy2: Update to 3.3.5

comment:78 Changed 2 years ago by Matthias Köppe

Summary: upgrade rpy2 package 2.8.2 -> 3.3.4, add new dependenciesupgrade rpy2 package 2.8.2 -> 3.3.5, add new dependencies

3.3.5 has a bugfix but did not backport https://github.com/rpy2/rpy2/pull/716, so we have to keep it as a patch.

comment:79 in reply to:  77 Changed 2 years ago by Emmanuel Charpentier

Replying to git:

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

4ce8970build/pkgs/rpy2: Update to 3.3.5

The mails emanating from Trac are not timely. So please don't update this branch just after asking for review... and please return state to needs_work before doing so.

comment:80 Changed 2 years ago by Matthias Köppe

Thanks for taking the time to test.

comment:81 Changed 2 years ago by Emmanuel Charpentier

  • Debian testing running on core i7 + 16 GB RAM
  • Sage configured to use system's R (4.0.2, as packaged by Debian)
  • Rebased on 9.2.beta5 (penibly : 5 merge conflicts...)

Compiles fine. Some rough tests in the Jupyter notebook.

sage -t --long --warn-long 194.3 src/sage/interfaces/r.py
    [257 tests, 4.80 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 4.9 seconds
    cpu time: 4.5 seconds
    cumulative wall time: 4.8 seconds

ptestlong underway.

Last edited 2 years ago by Emmanuel Charpentier (previous) (diff)

comment:82 in reply to:  81 Changed 2 years ago by Matthias Köppe

Replying to charpent:

  • Rebased on 9.2.beta5 (penibly : 5 merge conflicts...)

Thanks for pointing this out - I've just merged 9.2.beta5 into dependency #30118

comment:83 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:84 Changed 2 years ago by git

Commit: 4ce897074e0570143ec4642a91bf2d846f3136351cf262c4348678f6b5bcaaa61b0df11eb8159392

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

5d13238Merge tag '9.2.beta5' into t/30118/handle_sage_check_packages_in_build_make_makefile_in__not_sage_spkg
1cf262cMerge branch 't/30118/handle_sage_check_packages_in_build_make_makefile_in__not_sage_spkg' into t/29441/public/29441

comment:85 Changed 2 years ago by Matthias Köppe

Now it's on top of 9.2.beta5.

comment:86 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

comment:87 in reply to:  86 ; Changed 2 years ago by Emmanuel Charpentier

Replying to mkoeppe:

You did it. Again...

comment:88 Changed 2 years ago by Matthias Köppe

Do you have additional ways of testing the R interface, or thoughts how we could expand the automatic tests? The big problem with R integration in Sage is that essentially there is none - just a few basic tests in r.py.

comment:89 Changed 2 years ago by Matthias Köppe

By the way, you can verify that your rebased tree is identical to the one from my merge by doing git trac fetch 29441 && git reset FETCH_HEAD.

comment:90 Changed 2 years ago by Matthias Köppe

Automatic tests (of commit 4ce897074e, together with some other upgrade tickets) run at https://github.com/mkoeppe/sage/actions/runs/171722885

comment:91 in reply to:  87 ; Changed 2 years ago by Emmanuel Charpentier

Replying to charpent:

Replying to mkoeppe:

You did it. Again...

On Debian testing running on ore i7 + 16 GB RAM, after rebasing #29441 on 9.2.beta5, ptestlong gives 9 permanent (and no transient) failures :

| File                            | Result             | P/T | Notes            |
|---------------------------------+--------------------+-----+------------------|
| src/sage/doctest/test.py        | 23 doctests failed | P   | Ununderstandable |
| src/sage/plot/plot.py           | 1 doctest failed   | P   | Cosmetic ?       |
| src/sage/tests/cmdline.py       | 31 doctests failed | P   | Cosmetic ?       |
| src/sage/libs/ppl.pyx           | 9 doctests failed  | P   | Cosmetic ?       |
| src/sage/repl/interpreter.py    | 1 doctest failed   | P   | Cosmetic ?       |
| src/sage/rings/complex_arb.pyx  | 6 doctests failed  | P   | Known            |
| src/sage/misc/temporary_file.py | 1 doctest failed   | P   | Cosmetic ?       |
| src/sage/tests/gap_packages.py  | 1 doctest failed   | P   | Known, cosmetic  |
| src/sage/rings/real_arb.pyx     | 2 doctests failed  | P   | Known            |

FWIW, the same tests gave 3 permanent and one transient failures on 9.2.beta5 (identical to those reported for beta2 and beta4) :

| File                           | Result            | P/T |
|--------------------------------+-------------------+-----|
| src/sage/tests/parigp.py       | Timed out         | T   |
| src/sage/rings/complex_arb.pyx | 6 doctests failed | P   |
| src/sage/tests/gap_packages.py | 1 doctest failed  | P   |
| src/sage/rings/real_arb.pyx    | 2 doctests failed | P   |

Most of the "new" errors seems to be bound to a runtime redefinition of SAGE_ROOT, changing from /usr/local/sag to /usr/local/sage-9 (on my system, the former is a symlink to the latter, and /usr/local/bin/sage is a symlink to /usr/local/sage/sage ; this allows for a quick switch of Sage trees...).

I upload chkerrs.txt which is the log of re-running failed tests.

I'll retest the latest state of this branch when I have some credible indication that it is indeed intended to be stable...

Changed 2 years ago by Emmanuel Charpentier

Attachment: chkerrs.txt added

comment:92 Changed 2 years ago by Matthias Köppe

Thanks for testing. We have a fix for the symlink behavior in #29446. This is unrelated to the present ticket.

comment:93 in reply to:  88 Changed 2 years ago by Matthias Köppe

In case you did not see this:

Replying to mkoeppe:

Do you have additional ways of testing the R interface, or thoughts how we could expand the automatic tests? The big problem with R integration in Sage is that essentially there is none - just a few basic tests in r.py.

comment:94 Changed 2 years ago by Matthias Köppe

cygwin-standard build succeeded at https://github.com/mkoeppe/sage/runs/879259688

cygwin-minimal did not get to rpy2 because of unrelated problems

comment:95 in reply to:  91 Changed 2 years ago by Emmanuel Charpentier

Status: needs_reviewpositive_review

Replying to charpent:

[ Snip... ]

I'll retest the latest state of this branch when I have some credible indication that it is indeed intended to be stable...irmed by someone using Sage's interpreter

I now have done so, and get identical results.

My impression is that this upgrade doesn't bring new detectable bugs : the "new" bugs I have found seem unrelated.

==>positive_review.

However, I strongly advise a confirmation of this review by someone using Sage's R interpreter.

comment:96 Changed 2 years ago by Matthias Köppe

Thank you.

comment:97 Changed 2 years ago by Matthias Köppe

Reviewers: Emmanuel Charpentier, ...

comment:98 Changed 2 years ago by Volker Braun

Status: positive_reviewneeds_work
File "src/sage/stats/r.py", line 42, in sage.stats.r.ttest
Failed example:
    a, b = ttest([1,2,3,4,5],[1,2,3,3.5,5.121]); a # abs tol 1e-12
Exception raised:
    Traceback (most recent call last):
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 331, in _coerce_from_special_method
        return (x.__getattribute__(s))(self)
      File "sage/structure/element.pyx", line 493, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4644)
        return self.getattr_from_category(name)
      File "sage/structure/element.pyx", line 506, in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4756)
        return getattr_from_other_class(self, cls, name)
      File "sage/cpython/getattr.pyx", line 394, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2641)
        raise AttributeError(dummy_error_message)
    AttributeError: 'sage.rings.integer.Integer' object has no attribute '_r_'
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1139, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.stats.r.ttest[0]>", line 1, in <module>
        a, b = ttest([Integer(1),Integer(2),Integer(3),Integer(4),Integer(5)],[Integer(1),Integer(2),Integer(3),RealNumber('3.5'),RealNumber('5.121')]); a # abs tol 1e-12
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/stats/r.py", line 48, in ttest
        test = myR.t_test(x,y,conf_level = conf_level, **kw)._sage_()
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 2003, in __call__
        return self._parent.function_call(self._name, args=list(args), kwds=kwds)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 1077, in function_call
        args, kwds = self._convert_args_kwds(args, kwds)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 557, in _convert_args_kwds
        args[i] = self(arg)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 310, in __call__
        result = self._coerce_impl(x, use_special=False)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 558, in _coerce_impl
        return super(R, self)._coerce_impl(x, use_special=use_special)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 359, in _coerce_impl
        w = self(v)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 303, in __call__
        result = self._coerce_from_special_method(x)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 333, in _coerce_from_special_method
        return self(x._interface_init_())
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 296, in __call__
        return cls(self, x, name=name)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 718, in __init__
        self._name = parent._create(value, name=name)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/interface.py", line 501, in _create
        self.set(name, value)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 1126, in set
        out = self.eval(cmd)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 1338, in eval
        self._lazy_init()
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 541, in _lazy_init
        self._r_to_sage_converter = _setup_r_to_sage_converter()
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/sage/interfaces/r.py", line 367, in _setup_r_to_sage_converter
        from rpy2.robjects.conversion import Converter
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/rpy2/robjects/__init__.py", line 19, in <module>
        from rpy2.robjects.robject import RObjectMixin, RObject
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/rpy2/robjects/robject.py", line 58, in <module>
        class RObjectMixin(object):
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/rpy2/robjects/robject.py", line 70, in RObjectMixin
        __show = _get_exported_value('methods', 'show')
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/rpy2/rinterface_lib/conversion.py", line 44, in _
        cdata = function(*args, **kwargs)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.7/site-packages/rpy2/rinterface.py", line 624, in __call__
        raise embedded.RRuntimeError(_rinterface._geterrmessage())
    rpy2.rinterface_lib.embedded.RRuntimeError: Error in dyn.load(file, DLLpath = DLLpath, ...) : 
      unable to load shared object '/home/buildbot/slave/sage_git/build/local/lib/R/library/methods/libs/methods.so':
      libR.so: cannot open shared object file: No such file or directory

comment:99 Changed 2 years ago by Matthias Köppe

on what machine

comment:100 Changed 2 years ago by Volker Braun

Doesn't appear to be machin specific, e.g. http://build.sagemath.org/#/builders/35/builds/1

comment:101 Changed 2 years ago by Dima Pasechnik

maybe we only tested on libR coming from the system?

comment:102 in reply to:  101 ; Changed 2 years ago by Emmanuel Charpentier

Replying to dimpase:

maybe we only tested on libR coming from the system?

That's why I advised to test on a machine using Sage's R...

comment:103 in reply to:  100 Changed 2 years ago by Matthias Köppe

Replying to vbraun:

Doesn't appear to be machin specific, e.g. http://build.sagemath.org/#/builders/35/builds/1

The build log there is not helpful. rpy2 was not built in that run.

comment:104 in reply to:  101 Changed 2 years ago by Matthias Köppe

Replying to dimpase:

maybe we only tested on libR coming from the system?

Of course we test builds where R is built as an spkg. That's pretty much the point of the -minimal builds.

comment:105 Changed 2 years ago by Matthias Köppe

And I do see this error in a (from scratch) build for linuxmint-19-minimal at https://github.com/mkoeppe/sage/runs/878897239?check_suite_focus=true

comment:106 in reply to:  102 Changed 2 years ago by Matthias Köppe

Replying to charpent:

Replying to dimpase:

maybe we only tested on libR coming from the system?

That's why I advised to test on a machine using Sage's R...

Yes, you were right about this. We have a systematic problem with R in Sage. I think in the long run we will have to make R an optional or experimental package (a remedy that I brought up before, during the 9.1 cycle) -- unless developers step up and take over maintenance of the R / rpy2 spkgs in Sage.

comment:107 in reply to:  105 Changed 2 years ago by Matthias Köppe

Replying to mkoeppe:

And I do see this error in a (from scratch) build for linuxmint-19-minimal at https://github.com/mkoeppe/sage/runs/878897239?check_suite_focus=true

docker run -it docker.pkg.github.com/mkoeppe/sage/sage-docker-linuxmint-19-minimal-with-targets:9.1.rc4-68125-g83dfe657f5

comment:108 Changed 2 years ago by Matthias Köppe

# ldd /sage/local/lib/R/library/methods/libs/methods.so
	linux-vdso.so.1 (0x00007ffea0df9000)
	libR.so => not found
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f605284e000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f6052e49000)

comment:109 Changed 2 years ago by Matthias Köppe

Summary: upgrade rpy2 package 2.8.2 -> 3.3.5, add new dependenciesupgrade rpy2 package 2.8.2 -> 3.3.5, upgrade R to 3.6.3, add new dependencies

comment:110 Changed 2 years ago by git

Commit: 1cf262c4348678f6b5bcaaa61b0df11eb81593929d667e828a60c0782c60876d8b470d20b035d1c9

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

11448a6Update R to 3.6.3, add upstream_url
9d667e8build/pkgs/r/spkg-install.in: Set rpath

comment:111 Changed 2 years ago by git

Commit: 9d667e828a60c0782c60876d8b470d20b035d1c9314b15feb5e580628d77c11190d0118b1e15e746

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

a17c655m4/sage_spkg_collect.m4: Do not include script packages in SAGE_SDIST_PACKAGES
827923abootstrap: Accept 2020s in config.guess version check
c30ac54tox.ini: Add local-nobootstrap
de6464d.github/workflows/tox.yml: Add jobs "dist", "local-macos-nohomebrew
775b7f7Trac #30088: add a few explanatory comments to m4/sage_spkg_collect.m4.
160862fm4/sage_spkg_collect.m4: Mention sagelib in the comment
d1a2cbfMerge branch 't/30088/fix__make_sdist___add_test_run_to_github_actions' into t/29929/tox_ini__add_a_macos_environment_without_homebrew__conda
4398ea8Merge branch 'u/mkoeppe/tox_ini__add_a_macos_environment_without_homebrew__conda' of git://trac.sagemath.org/sage into t/29441/public/29441
f7213c5Trac #30149: When creating the Python venv on Cygwin make sure to copy
314b15fMerge branch 'u/embray/ticket-30149' of git://trac.sagemath.org/sage into t/29441/public/29441

comment:112 Changed 2 years ago by Matthias Köppe

Dependencies: #29851, #30064, #30118#29851, #30064, #30118, #30067, #30147, #29929, #30149

comment:113 Changed 2 years ago by Matthias Köppe

comment:114 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

comment:115 in reply to:  113 Changed 2 years ago by Matthias Köppe

Replying to mkoeppe:

Tests run at https://github.com/mkoeppe/sage/actions/runs/180231628

Tests look all clean now, including the -minimal platforms that failed previously.

Needs review.

comment:116 Changed 2 years ago by git

Commit: 314b15feb5e580628d77c11190d0118b1e15e74654d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c

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

54d0f99Merge tag '9.2.beta6' into t/29441/public/29441

comment:117 Changed 2 years ago by Dima Pasechnik

Reviewers: Emmanuel Charpentier, ...Emmanuel Charpentier, Dima Pasechnik
Status: needs_reviewpositive_review

looks ok

comment:118 Changed 2 years ago by Matthias Köppe

Thanks!

comment:119 Changed 2 years ago by Volker Braun

Branch: public/2944154d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c
Resolution: fixed
Status: positive_reviewclosed

comment:120 Changed 2 years ago by John Palmieri

Commit: 54d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c

This ticket causes some warning messages to be printed to the screen during doctesting:

% ./sage -t src/sage/interfaces/r.py
Running doctests with ID 2020-09-03-19-46-40-e4332149.
Git branch: t/29441/54d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c
Using --optional=build,dochtml,sage
Doctesting 1 file.
R[write to console]: Warning messages:

R[write to console]: 1: 
R[write to console]: In sage10 + sage6 :
R[write to console]: 
 
R[write to console]:  longer object length is not a multiple of shorter object length

R[write to console]: 2: 
R[write to console]: In sqrt(sage10) :
R[write to console]:  NaNs produced

R[write to console]: 3: 
R[write to console]: In sqrt(sage4) :
R[write to console]:  NaNs produced

sage -t --warn-long 83.1 --random-seed=0 src/sage/interfaces/r.py
    [257 tests, 3.44 s]
----------------------------------------------------------------------
All tests passed!

I think these should be silenced.

Note: See TracTickets for help on using tickets.