Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#25548 closed enhancement (fixed)

get rid of twisted reactor

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.3
Component: refactoring Keywords:
Cc: jdemeyer, embray, fbissey Merged in:
Authors: Frédéric Chapoton Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 0c577e5 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges


if possible. This does not seems to be used anywhere.

Change History (15)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/25548
  • Commit set to 0c577e506664c6dcc13a981c36014d099c5173ff
  • Status changed from new to needs_review

New commits:

0c577e5get rid of any reference to "twisted"

comment:2 Changed 3 years ago by jdemeyer

It's used by sagenb. I'm not sure whether that code in sage is needed though.

comment:3 Changed 3 years ago by fbissey

At the very least it creates a twisted dependency for sage that I was not aware off. If we can, we definitely should remove it.

comment:4 Changed 3 years ago by dimpase

should we perhaps instead downgrade sagenb to optional, and mark these tests as such?

comment:5 Changed 3 years ago by dimpase

#8785 appears to explain the reason for these tests.

Last edited 3 years ago by dimpase (previous) (diff)

comment:6 Changed 3 years ago by fbissey

I have seen stuff a bit like that before. This is annoying because it is hard for a normal dev to reproduce ( for example would require to build qt4 and some python-qt4 bindings inside a vanilla sage install before reproducing). If we still need to keep it, should we at least put the from twisted.internet import reactor in inside a try block so it doesn't cause problem if twisted is not installed?

comment:7 Changed 3 years ago by chapoton

The bot is green. I would be ready to take the risk of the removal..

comment:8 Changed 3 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Right let's wait for complaints after the 8.3 release :)

comment:9 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/25548 to 0c577e506664c6dcc13a981c36014d099c5173ff
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 3 years ago by embray

  • Commit 0c577e506664c6dcc13a981c36014d099c5173ff deleted

FWIW this sort of thing will less likely be a problem come the day we can fully switch to Python 3, where the multiprocessing module has been refactored to not use so much module-global state that interacts poorly when multiprocessing is used by multiple Python modules for different purposes within the same application.

comment:11 Changed 3 years ago by kcrisman

Apparently this breaks certain functionality for getting from the current default notebook back to sagenb. See #22431. Should we probably open a blocker to revert at least some of this?

comment:12 Changed 3 years ago by chapoton

which part needs to be reverted ?

comment:13 Changed 3 years ago by dimpase

I would rather propose to remove that "getting back" functionality, as non-essential.

comment:14 Changed 3 years ago by kcrisman

No, because the default is currently the Exporter Jeroen and/or Volker wrote. So in that case one would move straight to Jupyter. I don't know that sagenb has been deprecated at all properly w/ message, much less long enough for moving to only Jupyter. This is an easy fix.

comment:15 Changed 3 years ago by kcrisman

See #25667 as a followup.

Note: See TracTickets for help on using tickets.