#29320 closed defect (fixed)
downgrade various sagenb deps to optional
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  packages: standard  Keywords:  sagenb, dependencies, optional 
Cc:  slelievre, fbissey  Merged in:  
Authors:  Frédéric Chapoton, John Palmieri  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  d93d9d1 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
This includes twisted
and flask*
packages.
Change History (30)
comment:1 Changed 7 months ago by
 Branch set to u/chapoton/29320
 Commit set to 89d2a99c96a7197de4b15818692519939d5639f3
 Status changed from new to needs_review
comment:2 Changed 7 months ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
You beat me by 5 minutes :) But I've tested that this works.
comment:3 Changed 7 months ago by
comment:4 Changed 7 months ago by
 Cc slelievre added
 Description modified (diff)
 Keywords sagenb dependencies optional added
comment:5 Changed 7 months ago by
 Cc fbissey added
 Status changed from positive_review to needs_info
@fbissey told me that terminado uses twisted, so maybe twisted should be kept as standard. (Also, it should be listed in terminado's dependencies file, if this is accurate.) Can anyone confirm?
comment:6 Changed 7 months ago by
I should have said tornado, I misread the output of my dependency query in gentoo. That being said terminado depends on tornado.
comment:7 followup: ↓ 14 Changed 7 months ago by
the tests pass, and I manually checked dependencies to make sure that this change leads to no standard packages missing deps. If there are undeclared deps of untested standard packages, it is a bug.
comment:8 Changed 7 months ago by
Other candidates for downgrade: python_openid, speaklater
comment:9 Changed 7 months ago by
I think we can downgrade tornado and terminado etc. I don't know who is using them.
comment:10 followup: ↓ 11 Changed 7 months ago by
The Jupyter notebook may use these if they are available, but I don't know any details.
comment:11 in reply to: ↑ 10 Changed 7 months ago by
Replying to jhpalmieri:
The Jupyter notebook may use these if they are available, but I don't know any details.
They are actually mandatory https://github.com/jupyter/notebook/blob/master/setup.py#L99
comment:12 Changed 7 months ago by
Twisted, on the other hand, looks optional: see "Prerequisites" in https://www.tornadoweb.org/. Twisted is only used in tornado.platform.twisted
: "This module lets you run applications and libraries written for Twisted in a Tornado application."
comment:13 Changed 7 months ago by
So it looks to me like python_openid
, speaklater
, and twisted
could all be removed. Re twisted
especially, we should not only build and run tests, but also make sure the Jupyter notebook works.
comment:14 in reply to: ↑ 7 ; followup: ↓ 17 Changed 7 months ago by
Replying to dimpase:
the tests pass
Not for me:
$ ./sage t src/sage/combinat/root_system/cartan_type.py Running doctests with ID 2020031316274631297653. Git branch: t/29320/29320 Using optional=build,dochtml,sage Doctesting 1 file. sage t warnlong 69.1 src/sage/combinat/root_system/cartan_type.py ********************************************************************** File "src/sage/combinat/root_system/cartan_type.py", line 3069, in sage.combinat.root_system.cartan_type.CartanType_simple_finite.__setstate__ Failed example: pg_unpickleModule = unpickle_global('twisted.persisted.styles', 'unpickleModule') Exception raised: Traceback (most recent call last): File "sage/misc/persist.pyx", line 548, in sage.misc.persist.unpickle_global (build/cythonized/sage/misc/persist.c:5033) __import__(module) ModuleNotFoundError: No module named 'twisted' ...
comment:15 Changed 7 months ago by
In brief testing, though, the Jupyter notebook looks okay.
comment:16 Changed 7 months ago by
I propose these changes:

build/pkgs/python_openid/type
diff git a/build/pkgs/python_openid/type b/build/pkgs/python_openid/type index a6a7b9cd72..134d9bc32d 100644
a b 1 standard 1 optional 
build/pkgs/speaklater/type
diff git a/build/pkgs/speaklater/type b/build/pkgs/speaklater/type index a6a7b9cd72..134d9bc32d 100644
a b 1 standard 1 optional 
build/pkgs/twisted/type
diff git a/build/pkgs/twisted/type b/build/pkgs/twisted/type index 134d9bc32d..a6a7b9cd72 100644
a b 1 optional 1 standard
comment:17 in reply to: ↑ 14 ; followup: ↓ 18 Changed 7 months ago by
Replying to jhpalmieri:
Replying to dimpase:
the tests pass
Not for me:
$ ./sage t src/sage/combinat/root_system/cartan_type.py ...
Admittedly, this is a doctest for a sort of stupid function: "Implements the unpickling of Cartan types pickled by Sage <= 4.0."
comment:18 in reply to: ↑ 17 Changed 7 months ago by
Replying to jhpalmieri:
Replying to jhpalmieri:
Replying to dimpase:
the tests pass
Not for me:
$ ./sage t src/sage/combinat/root_system/cartan_type.py ...Admittedly, this is a doctest for a sort of stupid function: "Implements the unpickling of Cartan types pickled by Sage <= 4.0."
Oops, I overlooked this, I thought that this must be irrelevant.
Let's just tag it # optional  twisted
. I don't see a need to keep twisted
standard just cause of this.
comment:19 Changed 7 months ago by
 Branch changed from u/chapoton/29320 to u/jhpalmieri/29320
comment:20 Changed 7 months ago by
 Commit changed from 89d2a99c96a7197de4b15818692519939d5639f3 to d93d9d17eaff16185b810af629d65e78d17aad5d
Okay, ready for review. Please check the functionality of the Jupyter notebook; do we lose anything when we build without twisted?
New commits:
d93d9d1  trac 29320: make python_openid and speaklater optional.

comment:21 Changed 7 months ago by
 Status changed from needs_info to needs_review
comment:22 Changed 7 months ago by
Looking at:
$ cat build/pkgs/sagenb/dependencies $(STARTED)  pip babel flask flask_autoindex flask_babel flask_oldsessions flask_openid mathjax twisted sphinx  All lines of this file are ignored except the first. It is copied by SAGE_ROOT/build/make/install into SAGE_ROOT/build/make/Makefile.
Are there other dependencies to be removed from being standard? like babel ?
comment:23 Changed 7 months ago by
babel
should actually be in the list removed from standard I believe. Thanks for bringing it up. twisted
seems a bit ambiguous but mathjax
and sphinx
should definitely stay standard.
comment:24 followup: ↓ 25 Changed 7 months ago by
babel
is listed as a dependency of Sphinx, so it should be kept.
comment:25 in reply to: ↑ 24 Changed 7 months ago by
Replying to jhpalmieri:
babel
is listed as a dependency of Sphinx, so it should be kept.
Ah! Sorry, my mistake.
comment:26 Changed 7 months ago by
 Status changed from needs_review to positive_review
Looks good, but we need to take care of the dependency of that pickle on twisted (on a followup ticket).
comment:27 Changed 7 months ago by
I don't understand pickling, but I am tempted to make this change:

src/sage/combinat/root_system/cartan_type.py
diff git a/src/sage/combinat/root_system/cartan_type.py b/src/sage/combinat/root_system/cartan_type.py index 202e1abe11..3cac952462 100644
a b class CartanType_simple_finite(object): 3066 3066 3067 3067 sage: pg_CartanType_simple_finite = unpickle_global('sage.combinat.root_system.cartan_type', 'CartanType_simple_finite') 3068 3068 sage: si1 = unpickle_newobj(pg_CartanType_simple_finite, ()) 3069 sage: pg_unpickleModule = unpickle_global('twisted.persisted.styles', 'unpickleModule')3069 sage: from sage.misc.fpickle import unpickleModule 3070 3070 sage: pg_make_integer = unpickle_global('sage.rings.integer', 'make_integer') 3071 3071 sage: si2 = pg_make_integer('4') 3072 sage: unpickle_build(si1, {'tools': pg_unpickleModule('sage.combinat.root_system.type_A'), 't':['A', si2], 'letter':'A', 'n':si2})3072 sage: unpickle_build(si1, {'tools':unpickleModule('sage.combinat.root_system.type_A'), 't':['A', si2], 'letter':'A', 'n':si2}) 3073 3073 3074 3074 sage: si1 3075 3075 ['A', 4]
I'm building and running tests now.
comment:28 Changed 7 months ago by
See #29348 for the pickling issue.
comment:29 Changed 7 months ago by
 Branch changed from u/jhpalmieri/29320 to d93d9d17eaff16185b810af629d65e78d17aad5d
 Resolution set to fixed
 Status changed from positive_review to closed
comment:30 Changed 5 months ago by
 Commit d93d9d17eaff16185b810af629d65e78d17aad5d deleted
Followup at #29752: werkzeug can be optional, too.
here is a branch (not tested)
New commits:
change twisted and flask_* to optional packages