Opened 10 years ago

Closed 7 years ago

#11409 closed enhancement (fixed)

Remove old notebook files

Reported by: jdemeyer Owned by: jdemeyer
Priority: major Milestone: sage-5.13
Component: notebook Keywords:
Cc: fbissey, was, 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:

Description (last modified by jdemeyer)

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 jdemeyer 9 years ago.
Patch for local/bin
11409_sagenb.patch (5.2 KB) - added by jdemeyer 9 years ago.
Patch for SAGENB
11409.patch (11.3 KB) - added by jdemeyer 9 years ago.
Misc changes
trac_11409-rebased.patch (11.4 KB) - added by jhpalmieri 8 years ago.
rebased version of "11409.patch"
trac_11409-scripts.patch (934 bytes) - added by jhpalmieri 8 years ago.
modification of 11409_scripts.patch
trac_11409-rebased-v2.patch (13.2 KB) - added by chapoton 7 years ago.
rebased on 5.12.beta4
11409_doctest.patch (767 bytes) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (59)

comment:1 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 10 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:4 Changed 10 years ago by fbissey

  • Cc fbissey added

Changed 9 years ago by jdemeyer

Patch for local/bin

comment:5 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Changed 9 years ago by jdemeyer

Patch for SAGENB

comment:12 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_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 9 years ago by jdemeyer

  • Component changed from misc to notebook

comment:15 follow-up: Changed 9 years ago by 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). I don't know how accurate that is, but thought I should mention it.

comment:16 in reply to: ↑ 15 Changed 9 years ago by jdemeyer

  • Cc was 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 9 years ago by kcrisman

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 9 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-4.7.1 to sage-4.7.2
  • Owner changed from jason to jdemeyer
  • Priority changed from major to critical

comment:19 Changed 9 years ago by kini

  • Cc kini added

This was mentioned on sage-support.

comment:20 follow-up: Changed 9 years ago by kcrisman

  • Status changed from needs_review to needs_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 9 years ago by jdemeyer

  • Description modified (diff)

comment:22 Changed 9 years ago by jdemeyer

Rebased 11409.patch

Changed 9 years ago by jdemeyer

Misc changes

comment:23 in reply to: ↑ 20 Changed 9 years ago by jhpalmieri

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 9 years ago by kcrisman

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 9 years ago by jdemeyer

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 9 years ago by jdemeyer

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

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

comment:28 in reply to: ↑ 27 ; follow-up: Changed 8 years ago by was

Replying to jhpalmieri:

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

Yes!

comment:29 in reply to: ↑ 28 Changed 8 years ago by ppurka

Replying to was:

Replying to jhpalmieri:

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

Yes!

I see a recursion here. ;)

Changed 8 years ago by jhpalmieri

rebased version of "11409.patch"

Changed 8 years ago by jhpalmieri

modification of 11409_scripts.patch

comment:30 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

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

comment:31 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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

comment:32 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:33 Changed 8 years ago by jhpalmieri

I have created a pull request for the sagenb changes.

comment:34 Changed 8 years ago by jhpalmieri

  • Dependencies set to #13794
  • Description modified (diff)

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

comment:35 Changed 8 years ago by ppurka

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 8 years ago by kini

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

comment:37 Changed 8 years ago by ppurka

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 8 years ago by ohanar

  • Dependencies changed from #13794 to #13794, #14330
  • Description modified (diff)
  • Priority changed from critical to major

comment:39 Changed 7 years ago by kcrisman

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

comment:40 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

Changed 7 years ago by chapoton

rebased on 5.12.beta4

comment:41 Changed 7 years ago by 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 7 years ago by chapoton

apply only trac_11409-rebased-v2.patch

comment:43 Changed 7 years ago by chapoton

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

comment:44 Changed 7 years ago by ppurka

  • Reviewers set to Punarbasu Purkayastha
  • Status changed from needs_review to positive_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 7 years ago by jdemeyer

comment:45 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from positive_review to needs_review

comment:46 follow-up: Changed 7 years ago by 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.

comment:47 in reply to: ↑ 46 Changed 7 years ago by jdemeyer

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 follow-up: Changed 7 years ago by ppurka

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 ; follow-up: Changed 7 years ago by jdemeyer

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 7 years ago by ppurka

  • Status changed from needs_review to positive_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 7 years ago by ppurka

  • Reviewers changed from Punarbasu Purkayastha to Frédéric Chapoton, Punarbasu Purkayastha

comment:52 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.13.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.