Opened 19 months ago

Closed 15 months ago

Last modified 14 months 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: vdelecroix Owned by:
Priority: major Milestone: sage-9.2
Component: packages: standard Keywords:
Cc: mkoeppe, charpent, gh-timokau, isuruf, slelievre, arojas, embray, dimpase 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 mkoeppe)

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 charpent 15 months ago.
Doctest for src/sage/interfaces/r.py
chkerrs.txt (105.8 KB) - added by charpent 15 months ago.

Download all attachments as: .zip

Change History (122)

comment:1 Changed 19 months ago by vdelecroix

  • Cc charpent added
  • Description modified (diff)

The cygwin.patch does not longer apply.

comment:2 Changed 19 months ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 19 months ago by vdelecroix

  • Description modified (diff)

comment:4 follow-up: Changed 19 months ago by vdelecroix

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 19 months ago by vdelecroix

  • Description modified (diff)

comment:6 Changed 19 months ago by vdelecroix

Good news: tests pass on my machine!

Last edited 19 months ago by vdelecroix (previous) (diff)

comment:7 Changed 19 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:8 follow-ups: Changed 19 months ago by mkoeppe

See #28998 for pytest

comment:9 in reply to: ↑ 4 ; follow-up: Changed 19 months ago by charpent

  • Description modified (diff)
  • Milestone changed from sage-9.2 to sage-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 ; follow-up: Changed 19 months ago by vdelecroix

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 19 months ago by charpent

Replying to mkoeppe:

See #28998 for pytest

Dors it need much work ?

comment:12 in reply to: ↑ 10 Changed 19 months ago by charpent

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 19 months ago by vdelecroix

  • Branch set to public/29441
  • Commit set to c525fc2441b5a93f7c38e3c9bdff170b0454ef3b

New commits:

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

comment:14 follow-up: Changed 19 months ago by 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).

comment:15 in reply to: ↑ 9 ; follow-up: Changed 19 months ago by 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

comment:16 in reply to: ↑ 15 Changed 19 months ago by vdelecroix

  • Milestone changed from sage-9.1 to sage-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 19 months ago by vdelecroix

  • Description modified (diff)

comment:18 in reply to: ↑ 14 ; follow-ups: Changed 19 months ago by 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).

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 19 months ago by vdelecroix

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 follow-up: Changed 19 months ago by mkoeppe

  • 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 19 months ago by mkoeppe

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 19 months ago by mkoeppe

(See #29425 for an example)

comment:23 in reply to: ↑ 20 ; follow-up: Changed 19 months ago by vdelecroix

Replying to mkoeppe:

Description modified (diff)

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

comment:24 Changed 19 months ago by git

  • Commit changed from c525fc2441b5a93f7c38e3c9bdff170b0454ef3b to e672b6eef2b52c368c8288e33bdef57391be36b0

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

e672b6eadd upstream_url field to rpy2 checksums.ini

comment:25 Changed 19 months ago by vdelecroix

  • Description modified (diff)

comment:26 Changed 19 months ago by git

  • Commit changed from e672b6eef2b52c368c8288e33bdef57391be36b0 to 86d0d175c7e8d840ff6685910825f026b537f012

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

86d0d17let rpy2 not depend on pytest

comment:27 in reply to: ↑ 23 Changed 19 months ago by mkoeppe

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 19 months ago by git

  • Commit changed from 86d0d175c7e8d840ff6685910825f026b537f012 to 5eee83e9693a6e146b1a135329b8e6c153fe1288

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

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

comment:29 Changed 19 months ago by vdelecroix

  • Status changed from new to needs_review

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

comment:30 Changed 19 months ago by mkoeppe

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

comment:31 Changed 19 months ago by gh-timokau

  • Cc gh-timokau added

comment:32 Changed 18 months ago by vdelecroix

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 18 months ago by git

  • Commit changed from 5eee83e9693a6e146b1a135329b8e6c153fe1288 to 308d39c4ce3e477716aa4408def52c68c1416482

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

308d39cstill install tests along rpy2

comment:34 Changed 18 months ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Description modified (diff)

comment:35 Changed 18 months ago by mkoeppe

  • Status changed from needs_review to needs_work

Needs rebasing

comment:36 Changed 16 months ago by mkoeppe

#29813 adds pytest

comment:37 Changed 16 months ago by mkoeppe

  • Description modified (diff)

comment:38 Changed 16 months ago by mkoeppe

Latest rpy2 is now 3.3.3

comment:39 Changed 16 months ago by git

  • Commit changed from 308d39c4ce3e477716aa4408def52c68c1416482 to 81f580eff5007a666dc712e5219d47f5933e1573

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 16 months ago by mkoeppe

Rebased on 9.2.beta2

comment:41 Changed 16 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from upgrade rpy2 package 2.8.2 -> 3.2.7 to upgrade rpy2 package 2.8.2 -> 3.3.4, add new dependencies

comment:42 Changed 16 months ago by mkoeppe

  • Cc isuruf slelievre arojas added

comment:43 Changed 16 months ago by git

  • Commit changed from 81f580eff5007a666dc712e5219d47f5933e1573 to d4c16728171baa763dcb653f83aa485fdbeedefa

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 16 months ago by mkoeppe

  • Description modified (diff)

Patches need updating!

comment:46 Changed 16 months ago by mkoeppe

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Matthias Koeppe
  • Dependencies set to #29813

comment:47 Changed 16 months ago by git

  • Commit changed from d4c16728171baa763dcb653f83aa485fdbeedefa to d33797fe7e8f7a069a39eb446419592916fc90a9

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 16 months ago by mkoeppe

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 16 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:51 Changed 16 months ago by mkoeppe

  • Description modified (diff)

comment:52 Changed 16 months ago by mkoeppe

  • Description modified (diff)

comment:53 Changed 16 months ago by git

  • Commit changed from d33797fe7e8f7a069a39eb446419592916fc90a9 to b66e9df749993b975920a440f870bc3f6df19884

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

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

comment:54 Changed 16 months ago by mkoeppe

  • Dependencies #29813 deleted

comment:55 Changed 16 months ago by git

  • Commit changed from b66e9df749993b975920a440f870bc3f6df19884 to 4c7f18dfc607ebe77bce93ea1db14509fc7a5c10

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 16 months ago by mkoeppe

  • Dependencies set to #29851, #30064

comment:57 in reply to: ↑ 50 Changed 16 months ago by mkoeppe

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 15 months ago by mkoeppe

  • Dependencies changed from #29851, #30064 to #29851, #30064, #30118

comment:59 Changed 15 months ago by git

  • Commit changed from 4c7f18dfc607ebe77bce93ea1db14509fc7a5c10 to 3b14bf51517e2c8c77a49dcc286fe6dfaf38dc48

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 15 months ago by mkoeppe

  • Cc embray dimpase added

comment:61 follow-up: Changed 15 months ago by charpent

  • Status changed from needs_review to needs_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 15 months ago by charpent

Doctest for src/sage/interfaces/r.py

comment:62 in reply to: ↑ 61 Changed 15 months ago by charpent

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 15 months ago by dimpase

yeah, I see this error too.

comment:64 follow-up: Changed 15 months ago by dimpase

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 15 months ago by charpent

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 15 months ago by dimpase

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

comment:67 Changed 15 months ago by dimpase

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 15 months ago by mkoeppe

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

comment:69 Changed 15 months ago by mkoeppe

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

comment:70 Changed 15 months ago by mkoeppe

... and update package pytz to 2020.1

comment:71 Changed 15 months ago by mkoeppe

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

I'll prepare the pytz update first.

comment:72 Changed 15 months ago by git

  • Commit changed from 3b14bf51517e2c8c77a49dcc286fe6dfaf38dc48 to 5f93e7f059fcbfa3eecf1dd22112fb647498a506

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

5f93e7fbuild/pkgs/pytz: Update to 2020.1

comment:73 Changed 15 months ago by git

  • Commit changed from 5f93e7f059fcbfa3eecf1dd22112fb647498a506 to 091ed905e020537a18ff6f7c1251c87253537a0d

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 15 months ago by git

  • Commit changed from 091ed905e020537a18ff6f7c1251c87253537a0d to b01e92dd86b042c17d2bb17d44354d60bef861ab

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

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

comment:75 Changed 15 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:76 Changed 15 months ago by mkoeppe

  • Description modified (diff)

comment:77 follow-up: Changed 15 months ago by git

  • Commit changed from b01e92dd86b042c17d2bb17d44354d60bef861ab to 4ce897074e0570143ec4642a91bf2d846f313635

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

4ce8970build/pkgs/rpy2: Update to 3.3.5

comment:78 Changed 15 months ago by mkoeppe

  • Summary changed from upgrade rpy2 package 2.8.2 -> 3.3.4, add new dependencies to upgrade 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 15 months ago by charpent

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 15 months ago by mkoeppe

Thanks for taking the time to test.

comment:81 follow-up: Changed 15 months ago by charpent

  • 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 15 months ago by charpent (previous) (diff)

comment:82 in reply to: ↑ 81 Changed 15 months ago by mkoeppe

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 15 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:84 Changed 15 months ago by git

  • Commit changed from 4ce897074e0570143ec4642a91bf2d846f313635 to 1cf262c4348678f6b5bcaaa61b0df11eb8159392

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 15 months ago by mkoeppe

Now it's on top of 9.2.beta5.

comment:86 follow-up: Changed 15 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:87 in reply to: ↑ 86 ; follow-up: Changed 15 months ago by charpent

Replying to mkoeppe:

You did it. Again...

comment:88 follow-up: Changed 15 months ago by 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:89 Changed 15 months ago by mkoeppe

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 15 months ago by mkoeppe

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

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 15 months ago by charpent

comment:92 Changed 15 months ago by mkoeppe

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 15 months ago by mkoeppe

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 15 months ago by mkoeppe

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 15 months ago by charpent

  • Status changed from needs_review to positive_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 15 months ago by mkoeppe

Thank you.

comment:97 Changed 15 months ago by mkoeppe

  • Reviewers set to Emmanuel Charpentier, ...

comment:98 Changed 15 months ago by vbraun

  • Status changed from positive_review to needs_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 15 months ago by mkoeppe

on what machine

comment:100 follow-up: Changed 15 months ago by vbraun

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

comment:101 follow-ups: Changed 15 months ago by dimpase

maybe we only tested on libR coming from the system?

comment:102 in reply to: ↑ 101 ; follow-up: Changed 15 months ago by 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...

comment:103 in reply to: ↑ 100 Changed 15 months ago by mkoeppe

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 15 months ago by mkoeppe

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 follow-up: Changed 15 months ago by 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

comment:106 in reply to: ↑ 102 Changed 15 months ago by mkoeppe

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 15 months ago by mkoeppe

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 15 months ago by mkoeppe

# 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 15 months ago by mkoeppe

  • Summary changed from upgrade rpy2 package 2.8.2 -> 3.3.5, add new dependencies to upgrade rpy2 package 2.8.2 -> 3.3.5, upgrade R to 3.6.3, add new dependencies

comment:110 Changed 15 months ago by git

  • Commit changed from 1cf262c4348678f6b5bcaaa61b0df11eb8159392 to 9d667e828a60c0782c60876d8b470d20b035d1c9

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 15 months ago by git

  • Commit changed from 9d667e828a60c0782c60876d8b470d20b035d1c9 to 314b15feb5e580628d77c11190d0118b1e15e746

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 15 months ago by mkoeppe

  • Dependencies changed from #29851, #30064, #30118 to #29851, #30064, #30118, #30067, #30147, #29929, #30149

comment:114 Changed 15 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:115 in reply to: ↑ 113 Changed 15 months ago by mkoeppe

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 15 months ago by git

  • Commit changed from 314b15feb5e580628d77c11190d0118b1e15e746 to 54d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c

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

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

comment:117 Changed 15 months ago by dimpase

  • Reviewers changed from Emmanuel Charpentier, ... to Emmanuel Charpentier, Dima Pasechnik
  • Status changed from needs_review to positive_review

looks ok

comment:118 Changed 15 months ago by mkoeppe

Thanks!

comment:119 Changed 15 months ago by vbraun

  • Branch changed from public/29441 to 54d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:120 Changed 14 months ago by jhpalmieri

  • Commit 54d0f9927d3bd2b1b24a1ca3b2fb9c15a1df816c deleted

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.