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: |
Description (last modified by )
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:
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- trac_11409-rebased-v2.patch
- 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).
Attachments (7)
Change History (59)
comment:1 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 12 years ago by
Authors: | → Jeroen Demeyer |
---|---|
Description: | modified (diff) |
comment:4 Changed 12 years ago by
Cc: | François Bissey added |
---|
Changed 12 years ago by
Attachment: | 11409_scripts.patch added |
---|
comment:5 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:6 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:7 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:8 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:9 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:10 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:11 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:12 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:13 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Status: | new → 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 11 years ago by
Component: | misc → notebook |
---|
comment:15 follow-up: 16 Changed 11 years ago by
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 Changed 11 years ago by
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
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
Description: | modified (diff) |
---|---|
Milestone: | sage-4.7.1 → sage-4.7.2 |
Owner: | changed from Jason Grout to Jeroen Demeyer |
Priority: | major → critical |
comment:20 follow-up: 23 Changed 11 years ago by
Status: | needs_review → 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 11 years ago by
Description: | modified (diff) |
---|
comment:23 Changed 11 years ago by
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. -- WilliamSo 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
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
I think we should support the following situation:
- User has old version sage-x.y.z on his own laptop, runs Sage using the notebook interface.
- His worksheets are stored in
$DOT_SAGE
. - He upgrades Sage to version 5.1 (either by
sage -upgrade
or just reinstalling from scratch). - He starts up the new notebook.
- 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
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: 28 Changed 10 years ago by
See William's comment. Shall we just delete the code now?
comment:28 follow-up: 29 Changed 10 years ago by
comment:29 Changed 10 years ago by
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
Attachment: | trac_11409-rebased.patch added |
---|
rebased version of "11409.patch"
Changed 10 years ago by
Attachment: | trac_11409-scripts.patch added |
---|
modification of 11409_scripts.patch
comment:30 Changed 10 years ago by
Description: | modified (diff) |
---|
I had to modify 11409_scripts.patch to get sage -n
to work.
comment:31 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
Given William's comment, I am marking this as "needs review".
comment:32 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:34 Changed 10 years ago by
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
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
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
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
Dependencies: | #13794 → #13794, #14330 |
---|---|
Description: | modified (diff) |
Priority: | critical → major |
comment:39 Changed 9 years ago by
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
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:41 Changed 9 years ago by
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:43 Changed 9 years ago by
hello, the bot is green, so there is an opportunity to review this now !
comment:44 Changed 9 years ago by
Reviewers: | → Punarbasu Purkayastha |
---|---|
Status: | needs_review → 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 9 years ago by
Attachment: | 11409_doctest.patch added |
---|
comment:45 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Status: | positive_review → needs_review |
comment:46 follow-up: 47 Changed 9 years ago by
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 Changed 9 years ago by
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: 49 Changed 9 years ago by
Sorry, the new patch actually introduces the doctest failure. I still get the old result in my copy of 5.13.beta1.
comment:49 follow-up: 50 Changed 9 years ago by
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 Changed 9 years ago by
Status: | needs_review → 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 9 years ago by
Reviewers: | Punarbasu Purkayastha → Frédéric Chapoton, Punarbasu Purkayastha |
---|
comment:52 Changed 9 years ago by
Merged in: | → sage-5.13.beta2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Patch for local/bin