Opened 3 months ago

Last modified 8 hours ago

#29441 needs_review enhancement

upgrade rpy2 package 2.8.2 -> 3.3.4, 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 Merged in:
Authors: Vincent Delecroix, Matthias Koeppe Reviewers:
Report Upstream: N/A Work issues:
Branch: public/29441 (Commits) Commit: 3b14bf51517e2c8c77a49dcc286fe6dfaf38dc48
Dependencies: #29851, #30064, #30118 Stopgaps:

Description (last modified by mkoeppe)

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

Change History (59)

comment:1 Changed 3 months ago by vdelecroix

  • Cc charpent added
  • Description modified (diff)

The cygwin.patch does not longer apply.

comment:2 Changed 3 months ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 3 months ago by vdelecroix

  • Description modified (diff)

comment:4 follow-up: Changed 3 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 3 months ago by vdelecroix

  • Description modified (diff)

comment:6 Changed 3 months ago by vdelecroix

Good news: tests pass on my machine!

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

comment:7 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

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

See #28998 for pytest

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

Replying to mkoeppe:

See #28998 for pytest

Dors it need much work ?

comment:12 in reply to: ↑ 10 Changed 3 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 3 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 3 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 3 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 3 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 3 months ago by vdelecroix

  • Description modified (diff)

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

(See #29425 for an example)

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

Replying to mkoeppe:

Description modified (diff)

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

comment:24 Changed 3 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 3 months ago by vdelecroix

  • Description modified (diff)

comment:26 Changed 3 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 3 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 3 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 3 months ago by vdelecroix

  • Status changed from new to needs_review

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

comment:30 Changed 3 months ago by mkoeppe

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

comment:31 Changed 3 months ago by gh-timokau

  • Cc gh-timokau added

comment:32 Changed 3 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 3 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 3 months ago by vdelecroix

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

comment:35 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

Needs rebasing

comment:36 Changed 4 weeks ago by mkoeppe

#29813 adds pytest

comment:37 Changed 4 weeks ago by mkoeppe

  • Description modified (diff)

comment:38 Changed 4 weeks ago by mkoeppe

Latest rpy2 is now 3.3.3

comment:39 Changed 9 days 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 9 days ago by mkoeppe

Rebased on 9.2.beta2

comment:41 Changed 9 days 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 9 days ago by mkoeppe

  • Cc isuruf slelievre arojas added

comment:43 Changed 9 days 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 9 days ago by mkoeppe

  • Description modified (diff)

Patches need updating!

comment:46 Changed 8 days ago by mkoeppe

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

comment:47 Changed 8 days 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 8 days 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 8 days ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:50 follow-up: Changed 8 days ago by mkoeppe

comment:51 Changed 8 days ago by mkoeppe

  • Description modified (diff)

comment:52 Changed 8 days ago by mkoeppe

  • Description modified (diff)

comment:53 Changed 8 days 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 4 days ago by mkoeppe

  • Dependencies #29813 deleted

comment:55 Changed 4 days 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 4 days ago by mkoeppe

  • Dependencies set to #29851, #30064

comment:57 in reply to: ↑ 50 Changed 4 days 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 8 hours ago by mkoeppe

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

comment:59 Changed 8 hours 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
Note: See TracTickets for help on using tickets.