Opened 9 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 )
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 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
- Cc fbissey added
Changed 9 years ago by
comment:5 Changed 9 years ago by
- Description modified (diff)
comment:6 Changed 9 years ago by
- Description modified (diff)
comment:7 Changed 9 years ago by
- Description modified (diff)
comment:8 Changed 9 years ago by
- Description modified (diff)
comment:9 Changed 9 years ago by
- Description modified (diff)
comment:10 Changed 9 years ago by
- Description modified (diff)
comment:11 Changed 9 years ago by
- Description modified (diff)
comment:12 Changed 9 years ago by
- Description modified (diff)
comment:13 Changed 9 years ago by
- 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
- Component changed from misc to notebook
comment:15 follow-up: ↓ 16 Changed 9 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 in reply to: ↑ 15 Changed 9 years ago by
- 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
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
- 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:20 follow-up: ↓ 23 Changed 8 years ago by
- 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 8 years ago by
- Description modified (diff)
comment:22 Changed 8 years ago by
Rebased 11409.patch
comment:23 in reply to: ↑ 20 Changed 8 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 8 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 8 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 8 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 8 years ago by
See William's comment. Shall we just delete the code now?
comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 8 years ago by
comment:29 in reply to: ↑ 28 Changed 8 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. ;)
comment:30 Changed 7 years ago by
- Description modified (diff)
I had to modify 11409_scripts.patch to get sage -n
to work.
comment:31 Changed 7 years ago by
- Status changed from needs_work to needs_review
Given William's comment, I am marking this as "needs review".
comment:32 Changed 7 years ago by
- Description modified (diff)
comment:33 Changed 7 years ago by
I have created a pull request for the sagenb changes.
comment:34 Changed 7 years ago by
- 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 7 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 7 years ago by
ppurka: So, positive review? If so, I'll merge jhpalmieri's pull request and include it in #14330.
comment:37 Changed 7 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 7 years ago by
- Dependencies changed from #13794 to #13794, #14330
- Description modified (diff)
- Priority changed from critical to major
comment:39 Changed 7 years ago by
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
- Milestone changed from sage-5.11 to sage-5.12
comment:41 Changed 7 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:42 Changed 7 years ago by
apply only trac_11409-rebased-v2.patch
comment:43 Changed 7 years ago by
hello, the bot is green, so there is an opportunity to review this now !
comment:44 Changed 7 years ago by
- 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
comment:45 Changed 7 years ago by
- Description modified (diff)
- Status changed from positive_review to needs_review
comment:46 follow-up: ↓ 47 Changed 7 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 in reply to: ↑ 46 Changed 7 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 7 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 in reply to: ↑ 48 ; follow-up: ↓ 50 Changed 7 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 in reply to: ↑ 49 Changed 7 years ago by
- 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
- Reviewers changed from Punarbasu Purkayastha to Frédéric Chapoton, Punarbasu Purkayastha
comment:52 Changed 7 years ago by
- Merged in set to sage-5.13.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
Patch for local/bin