Opened 7 months ago

Closed 7 months ago

Last modified 5 months ago

#29320 closed defect (fixed)

downgrade various sagenb deps to optional

Reported by: dimpase Owned by:
Priority: major Milestone: sage-9.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 slelievre)

This includes twisted and flask* packages.

Change History (30)

comment:1 Changed 7 months ago by chapoton

  • Branch set to u/chapoton/29320
  • Commit set to 89d2a99c96a7197de4b15818692519939d5639f3
  • Status changed from new to needs_review

here is a branch (not tested)


New commits:

89d2a99change twisted and flask_* to optional packages

comment:2 Changed 7 months ago by dimpase

  • Authors Dima Pasechnik deleted
  • 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 dimpase

  • Authors set to Frédéric Chapoton

comment:4 Changed 7 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)
  • Keywords sagenb dependencies optional added

comment:5 Changed 7 months ago by jhpalmieri

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

I should have said tornado, I misread the output of my dependency query in gentoo. That being said terminado depends on tornado.

comment:7 follow-up: Changed 7 months ago by dimpase

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.

Last edited 7 months ago by dimpase (previous) (diff)

comment:8 Changed 7 months ago by arojas

Other candidates for downgrade: python_openid, speaklater

comment:9 Changed 7 months ago by dimpase

I think we can downgrade tornado and terminado etc. I don't know who is using them.

comment:10 follow-up: Changed 7 months ago by jhpalmieri

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 arojas

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 jhpalmieri

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 jhpalmieri

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

Replying to dimpase:

the tests pass

Not for me:

$ ./sage -t src/sage/combinat/root_system/cartan_type.py
Running doctests with ID 2020-03-13-16-27-46-31297653.
Git branch: t/29320/29320
Using --optional=build,dochtml,sage
Doctesting 1 file.
sage -t --warn-long 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 jhpalmieri

In brief testing, though, the Jupyter notebook looks okay.

comment:16 Changed 7 months ago by jhpalmieri

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
     1optional
  • build/pkgs/speaklater/type

    diff --git a/build/pkgs/speaklater/type b/build/pkgs/speaklater/type
    index a6a7b9cd72..134d9bc32d 100644
    a b  
    1 standard
     1optional
  • build/pkgs/twisted/type

    diff --git a/build/pkgs/twisted/type b/build/pkgs/twisted/type
    index 134d9bc32d..a6a7b9cd72 100644
    a b  
    1 optional
     1standard

comment:17 in reply to: ↑ 14 ; follow-up: Changed 7 months ago by 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."

comment:18 in reply to: ↑ 17 Changed 7 months ago by dimpase

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 jhpalmieri

  • Branch changed from u/chapoton/29320 to u/jhpalmieri/29320

comment:20 Changed 7 months ago by jhpalmieri

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

d93d9d1trac 29320: make python_openid and speaklater optional.

comment:21 Changed 7 months ago by jhpalmieri

  • Status changed from needs_info to needs_review

comment:22 Changed 7 months ago by slabbe

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 fbissey

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

babel is listed as a dependency of Sphinx, so it should be kept.

comment:25 in reply to: ↑ 24 Changed 7 months ago by fbissey

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 dimpase

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, John Palmieri
  • 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 jhpalmieri

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): 
    30663066
    30673067            sage: pg_CartanType_simple_finite = unpickle_global('sage.combinat.root_system.cartan_type', 'CartanType_simple_finite')
    30683068            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
    30703070            sage: pg_make_integer = unpickle_global('sage.rings.integer', 'make_integer')
    30713071            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})
    30733073
    30743074            sage: si1
    30753075            ['A', 4]

I'm building and running tests now.

comment:28 Changed 7 months ago by jhpalmieri

See #29348 for the pickling issue.

comment:29 Changed 7 months ago by vbraun

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

  • Commit d93d9d17eaff16185b810af629d65e78d17aad5d deleted

Followup at #29752: werkzeug can be optional, too.

Note: See TracTickets for help on using tickets.