Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
videlec opened this issue Mar 31, 2020 · 139 comments
Closed

Comments

@videlec
Copy link
Contributor

videlec commented Mar 31, 2020

Main changes:

  • https://pypi.org/project/rpy2/, supports python >= 3.6

  • cygwin.patch does not apply anymore

  • cffi>=1.13.1 is a new dependency, we install latest 1.14.0 (supports python >= 3.2)

  • pycparser is a new dependency (required by cffi, supports python >= 3.4)

  • https://pypi.org/project/tzlocal/ is a new dependency, we install latest 2.1 (supports python >= 2.7)

  • https://pypi.org/project/pytz/ is a new dependency, we update to latest 2020.1 (supports python >= 2.4)

  • 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

Depends on #29851
Depends on #30064
Depends on #30118
Depends on #30067
Depends on #30147
Depends on #29929
Depends on #30149

CC: @mkoeppe @EmmanuelCharpentier @timokau @isuruf @slel @antonio-rojas @embray @dimpase

Component: packages: standard

Author: Vincent Delecroix, Matthias Koeppe

Branch: 54d0f99

Reviewer: Emmanuel Charpentier, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/29441

@videlec videlec added this to the sage-9.1 milestone Mar 31, 2020
@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

comment:1

The cygwin.patch does not longer apply.

@videlec

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

comment:4

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.

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

comment:6

Good news: tests pass on my machine!

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Mar 31, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 31, 2020

comment:8

See #28998 for pytest

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Mar 31, 2020

comment:9

Replying to @videlec:

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.

@EmmanuelCharpentier

This comment has been minimized.

@EmmanuelCharpentier EmmanuelCharpentier mannequin modified the milestones: sage-9.2, sage-9.1 Mar 31, 2020
@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

comment:10

Replying to @mkoeppe:

See #28998 for pytest

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

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Mar 31, 2020

comment:11

Replying to @mkoeppe:

See #28998 for pytest

Dors it need much work ?

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Mar 31, 2020

comment:12

Replying to @videlec:

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

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

New commits:

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

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

Branch: public/29441

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

Commit: c525fc2

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

comment:14

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 31, 2020

comment:15

Replying to @EmmanuelCharpentier:

Milestone changed from sage-9.2 to sage-9.1

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

@videlec
Copy link
Contributor Author

videlec commented Mar 31, 2020

comment:16

Replying to @mkoeppe:

Replying to @EmmanuelCharpentier:

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.

@videlec videlec modified the milestones: sage-9.1, sage-9.2 Mar 31, 2020
@videlec

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Jul 23, 2020

comment:101

maybe we only tested on libR coming from the system?

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Jul 23, 2020

comment:102

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:103

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:104

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:105

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:106

Replying to @EmmanuelCharpentier:

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:107

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:108
# 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)

@mkoeppe mkoeppe changed the title upgrade rpy2 package 2.8.2 -> 3.3.5, add new dependencies upgrade rpy2 package 2.8.2 -> 3.3.5, upgrade R to 3.6.3, add new dependencies Jul 23, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2020

Changed commit from 1cf262c to 9d667e8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2020

Changed commit from 9d667e8 to 314b15f

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

Changed dependencies from #29851, #30064, #30118 to #29851, #30064, #30118, #30067, #30147, #29929, #30149

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 23, 2020

comment:113

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 25, 2020

comment:115

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2020

Changed commit from 314b15f to 54d0f99

@dimpase
Copy link
Member

dimpase commented Jul 26, 2020

Changed reviewer from Emmanuel Charpentier, ... to Emmanuel Charpentier, Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jul 26, 2020

comment:117

looks ok

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 26, 2020

comment:118

Thanks!

@vbraun
Copy link
Member

vbraun commented Aug 2, 2020

Changed branch from public/29441 to 54d0f99

@jhpalmieri
Copy link
Member

comment:120

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.

@jhpalmieri
Copy link
Member

Changed commit from 54d0f99 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants