Opened 12 years ago

Closed 9 years ago

#11409 closed enhancement (fixed)

Remove old notebook files

Reported by: Jeroen Demeyer Owned by: Jeroen Demeyer
Priority: major Milestone: sage-5.13
Component: notebook Keywords:
Cc: François Bissey, William Stein, Keshav Kini Merged in: sage-5.13.beta2
Authors: Jeroen Demeyer Reviewers: Frédéric Chapoton, Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13794, #14330 Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

The old Sage notebook (as part of the Sage library) has been superseded by the sagenb spkg. The following files are from the old notebook and should be removed:

  • $SAGE_ROOT/devel/sage/sage/server/introspect.py
  • $SAGE_ROOT/devel/sage/sage/server/notebook/*
  • $SAGE_ROOT/devel/sage/sage/server/simple/*

The following files are duplicated in sagenb but contain some functionality which is not specific to the notebook. At the moment no effort is made to dis-entangle these (but I have made them more uniform)

  • $SAGE_ROOT/devel/sage/sage/server/misc.py
  • $SAGE_ROOT/devel/sage/sage/server/support.py

Apply:

  1. cd sage/server && hg remove introspect.py notebook simple && hg commit -m "Trac #11409: Remove old notebook files" # I prefer this shell command to a patch because there are no chances of a merge conflict
  2. trac_11409-rebased-v2.patch
  3. 11409_doctest.patch

After applying the patches, running sdist, and then make from scratch, I get the following results:

$ cd $SAGE_ROOT
$ grep -e 'server.simple' -e 'server.notebook' -e 'server.introspect' -R .
Binary file ./devel/sage/.hg/store/data/doc/en/reference/notebook.rst.i matches
Binary file ./devel/sage/.hg/store/data/_m_a_n_i_f_e_s_t.in.i matches
Binary file ./devel/sage/.hg/store/00manifest.d matches
Binary file ./devel/sage-main/.hg/store/data/doc/en/reference/notebook.rst.i matches
Binary file ./devel/sage-main/.hg/store/data/_m_a_n_i_f_e_s_t.in.i matches
Binary file ./devel/sage-main/.hg/store/00manifest.d matches
./local/share/texmf/tex/generic/sagetex/remote-sagetex.dtx:% in \texttt{sage/server/simple/twist.py}.

This proves that the deleted files are no longer referenced (expect for a comment in SageTeX, this has been reported to Dan Drake).


See also #9237, #9552.

Attachments (7)

11409_scripts.patch (930 bytes) - added by Jeroen Demeyer 12 years ago.
Patch for local/bin
11409_sagenb.patch (5.2 KB) - added by Jeroen Demeyer 11 years ago.
Patch for SAGENB
11409.patch (11.3 KB) - added by Jeroen Demeyer 11 years ago.
Misc changes
trac_11409-rebased.patch (11.4 KB) - added by John Palmieri 10 years ago.
rebased version of "11409.patch"
trac_11409-scripts.patch (934 bytes) - added by John Palmieri 10 years ago.
modification of 11409_scripts.patch
trac_11409-rebased-v2.patch (13.2 KB) - added by Frédéric Chapoton 9 years ago.
rebased on 5.12.beta4
11409_doctest.patch (767 bytes) - added by Jeroen Demeyer 9 years ago.

Download all attachments as: .zip

Change History (59)

comment:1 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:3 Changed 12 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Description: modified (diff)

comment:4 Changed 12 years ago by François Bissey

Cc: François Bissey added

Changed 12 years ago by Jeroen Demeyer

Attachment: 11409_scripts.patch added

Patch for local/bin

comment:5 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:6 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:7 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:8 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:9 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:10 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:11 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

Changed 11 years ago by Jeroen Demeyer

Attachment: 11409_sagenb.patch added

Patch for SAGENB

comment:12 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:13 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)
Status: newneeds_review

After applying the patches, running sdist, and then make from scratch, I get the following results:

$ cd $SAGE_ROOT
$ grep -e 'server.simple' -e 'server.notebook' -e 'server.introspect' -R .
Binary file ./devel/sage/.hg/store/data/doc/en/reference/notebook.rst.i matches
Binary file ./devel/sage/.hg/store/data/_m_a_n_i_f_e_s_t.in.i matches
Binary file ./devel/sage/.hg/store/00manifest.d matches
Binary file ./devel/sage-main/.hg/store/data/doc/en/reference/notebook.rst.i matches
Binary file ./devel/sage-main/.hg/store/data/_m_a_n_i_f_e_s_t.in.i matches
Binary file ./devel/sage-main/.hg/store/00manifest.d matches
./local/share/texmf/tex/generic/sagetex/remote-sagetex.dtx:% in \texttt{sage/server/simple/twist.py}.

This proves that the deleted files are no longer referenced (expect for a comment in SageTeX, this has been reported to Dan Drake).

comment:14 Changed 11 years ago by Jeroen Demeyer

Component: miscnotebook

comment:15 Changed 11 years ago by Karl-Dieter Crisman

Just as a comment, William made some reference at Sage Days 31 to a deprecation period and that these files were needed to enable people to migrate to the new notebook (not the really new flask notebook, nor the old new notebook, but the medium new separate sagenb notebook, I think). I don't know how accurate that is, but thought I should mention it.

comment:16 in reply to:  15 Changed 11 years ago by Jeroen Demeyer

Cc: William Stein added

Replying to kcrisman:

Just as a comment, William made some reference at Sage Days 31 to a deprecation period and that these files were needed to enable people to migrate to the new notebook (not the really new flask notebook, nor the old new notebook, but the medium new separate sagenb notebook, I think).

How long should we wait then? sagenb has existed at least since sage-4.2, released 25 october 2009 (almost 2 years ago). I think that is sufficient time.

comment:17 Changed 11 years ago by Karl-Dieter Crisman

That long, eh?

Anyway, since he mentioned it, I figure it would be worth asking him, since he mentioned it. Being a BDFL has its privileges :)

To be fair, I don't feel like I can review this in any case; it would have to be someone pretty familiar with the mechanics of the change to sagenb. But it would be really nice to take care of this, you are right.

comment:18 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)
Milestone: sage-4.7.1sage-4.7.2
Owner: changed from Jason Grout to Jeroen Demeyer
Priority: majorcritical

comment:19 Changed 11 years ago by Keshav Kini

Cc: Keshav Kini added

This was mentioned on sage-support.

comment:20 Changed 11 years ago by Karl-Dieter Crisman

Status: needs_reviewneeds_work

From this sage-notebook thread:

More importantly, it would be very useful if when we remove the code 
from devel/sage/sage/server that is needed to migrate, we include an 
error message that says: "Please migrate this Sage notebook server 
install by installing a version of Sage between 4.3 and 4.8, 
inclusive." 
Incidentally, I also plan to support the format of the sage notebook 
server files from 4.3 indefinitely.  It doesn't involve any version 
specific pickles, etc. 
 -- William 

So I guess something here about this.

comment:21 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:22 Changed 11 years ago by Jeroen Demeyer

Rebased 11409.patch

Changed 11 years ago by Jeroen Demeyer

Attachment: 11409.patch added

Misc changes

comment:23 in reply to:  20 Changed 11 years ago by John Palmieri

Replying to kcrisman:

From this sage-notebook thread:

More importantly, it would be very useful if when we remove the code 
from devel/sage/sage/server that is needed to migrate, we include an 
error message that says: "Please migrate this Sage notebook server 
install by installing a version of Sage between 4.3 and 4.8, 
inclusive." 
Incidentally, I also plan to support the format of the sage notebook 
server files from 4.3 indefinitely.  It doesn't involve any version 
specific pickles, etc. 
 -- William 

So I guess something here about this.

I'm not clear on what is required. Where would this error message appear? During an upgrade, or when running Sage? Or should we just add something to the top-level README file?

comment:24 Changed 11 years ago by Karl-Dieter Crisman

I would imagine ideally on an upgrade (that is, attempting to use a too-new Sage with too-old notebook files) it would "detect" super-old notebook files somehow, perhaps because they ask to use these files? The readme might be too subtle of a solution.

comment:25 Changed 11 years ago by Jeroen Demeyer

I think we should support the following situation:

  1. User has old version sage-x.y.z on his own laptop, runs Sage using the notebook interface.
  2. His worksheets are stored in $DOT_SAGE.
  3. He upgrades Sage to version 5.1 (either by sage -upgrade or just reinstalling from scratch).
  4. He starts up the new notebook.
  5. He finds his worksheets are still there. Maybe some of his old code doesn't work anymore, but at least it should be there.

We should support this for as old versions as possible.

comment:26 Changed 11 years ago by Jeroen Demeyer

Another thing is opening .sws files, but William said that the format for these hasn't changed since long. So that should be less of a worry.

comment:27 Changed 10 years ago by John Palmieri

See William's comment. Shall we just delete the code now?

comment:28 in reply to:  27 ; Changed 10 years ago by William Stein

Replying to jhpalmieri:

See William's comment. Shall we just delete the code now?

Yes!

comment:29 in reply to:  28 Changed 10 years ago by Punarbasu Purkayastha

Replying to was:

Replying to jhpalmieri:

See William's comment. Shall we just delete the code now?

Yes!

I see a recursion here. ;)

Changed 10 years ago by John Palmieri

Attachment: trac_11409-rebased.patch added

rebased version of "11409.patch"

Changed 10 years ago by John Palmieri

Attachment: trac_11409-scripts.patch added

modification of 11409_scripts.patch

comment:30 Changed 10 years ago by John Palmieri

Description: modified (diff)

I had to modify 11409_scripts.patch to get sage -n to work.

comment:31 Changed 10 years ago by John Palmieri

Status: needs_workneeds_review

Given William's comment, I am marking this as "needs review".

comment:32 Changed 10 years ago by John Palmieri

Description: modified (diff)

comment:33 Changed 10 years ago by John Palmieri

I have created a pull request for the sagenb changes.

comment:34 Changed 10 years ago by John Palmieri

Dependencies: #13794
Description: modified (diff)

I moved the scripts patch to #13794 and made that ticket a prerequisite for this one.

comment:35 Changed 10 years ago by Punarbasu Purkayastha

The patches (except the sagenb patch which doesn't apply to sagenb-main in 5.5rc0) + the changes in the pull request work. I tested interacts, plots, jmol and all of them are working.

comment:36 Changed 10 years ago by Keshav Kini

ppurka: So, positive review? If so, I'll merge jhpalmieri's pull request and include it in #14330.

comment:37 Changed 10 years ago by Punarbasu Purkayastha

It *was* positive review from me a couple of months ago. I will check it again later today to see that nothing is broken.

comment:38 Changed 10 years ago by R. Andrew Ohana

Dependencies: #13794#13794, #14330
Description: modified (diff)
Priority: criticalmajor

comment:39 Changed 9 years ago by Karl-Dieter Crisman

Note that jhpalmieri's pull request was indeed merged and might even be in Sage now, I think.

comment:40 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

Changed 9 years ago by Frédéric Chapoton

Attachment: trac_11409-rebased-v2.patch added

rebased on 5.12.beta4

comment:41 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

I have rebased the patch on 5.12.beta4

I had also to put back the import of alarm and cancel_alarm in server/misc.py

for the bot: apply only trac_11409-rebased-v2.patch

comment:42 Changed 9 years ago by Frédéric Chapoton

apply only trac_11409-rebased-v2.patch

comment:43 Changed 9 years ago by Frédéric Chapoton

hello, the bot is green, so there is an opportunity to review this now !

comment:44 Changed 9 years ago by Punarbasu Purkayastha

Reviewers: Punarbasu Purkayastha
Status: needs_reviewpositive_review

This looks fine to me. Irrespective of what the bot says, it applies just fine to 5.13.beta1. I tested 3D plots, 2D plots, debug() functionality, and interacts, and they are all working.

Changed 9 years ago by Jeroen Demeyer

Attachment: 11409_doctest.patch added

comment:45 Changed 9 years ago by Jeroen Demeyer

Description: modified (diff)
Status: positive_reviewneeds_review

comment:46 Changed 9 years ago by Punarbasu Purkayastha

Why didn't the patchbot catch this error earlier? I had only a laptop to run all doctests, so I was expecting the patchbot to report any such problems.

comment:47 in reply to:  46 Changed 9 years ago by Jeroen Demeyer

Replying to ppurka:

Why didn't the patchbot catch this error earlier? I had only a laptop to run all doctests, so I was expecting the patchbot to report any such problems.

I guess the patchbot doesn't understand the "apply" instruction

cd sage/server && hg remove introspect.py notebook simple && hg commit -m "Trac #11409: Remove old notebook files

comment:48 Changed 9 years ago by Punarbasu Purkayastha

Sorry, the new patch actually introduces the doctest failure. I still get the old result in my copy of 5.13.beta1.

comment:49 in reply to:  48 ; Changed 9 years ago by Jeroen Demeyer

Replying to ppurka:

I still get the old result in my copy of 5.13.beta1.

Did you actually remove the old notebook files?

comment:50 in reply to:  49 Changed 9 years ago by Punarbasu Purkayastha

Status: needs_reviewpositive_review

Replying to jdemeyer:

Did you actually remove the old notebook files?

Thanks! I rechecked everything in the notebook after trying this. It works fine, and also doctests fine.

comment:51 Changed 9 years ago by Punarbasu Purkayastha

Reviewers: Punarbasu PurkayasthaFrédéric Chapoton, Punarbasu Purkayastha

comment:52 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.13.beta2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.