Notebook keeps directories for deleted cells

[kcrisman]

If you delete a cell, it doesn't delete the directory for the cell, at least not very soon (it does seem that they eventually get deleted). Should it maybe when you log off the worksheet? Or maybe immediately. This is particularly bad when there are large computations or graphics involved, and may contribute to making .sws files rather large even if there are few cells.

Apply attachment: trac_5177-delete-cell-dirs.4.patch

CC: @dandrake

Component: notebook

Author: Tim Dumol, Mitesh Patel

Reviewer: Jason Grout

Merged: sage-4.8.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/5177

[sagetrac-mabshoff]

comment:1

3.4 is for ReST tickets only.

Cheers,

Michael

[TimDumol]

Attachment: trac_5177-delete-cell-dirs.patch.gz

Uses twisted.internet.threads.deferToThread to delete cell directory on cell delete. Should apply cleanly on sagenb-0.5.0.

[TimDumol]

comment:2

This hopefully won't cause any performance problems. Anyone mind giving their opinion on this?

[TimDumol]

Author: Tim Dumol

[TimDumol]

Attachment: trac_5177-delete-cell-dirs.2.patch.gz

Trivial rebase on new patch queue. (Deletion of one empty line)

[TimDumol]

comment:3

This is then new patch queue:

trac_7650-sagenb_doctesting_v6.patch
trac_7650-reviewer.patch
trac_7648-missing_indent.patch
trac_7663-rstrip_prompt.patch
trac_7847-empty-trash-no-referer.patch
trac_7786-template-jinja-idiomatic.patch
trac_7863-declare_vars_aux_js_v2.patch
trac_7874-typeset_interact_labels.patch
trac_7858-key_binding_vars_v2.patch
trac_7666-alphanumeric_cell_ids_B5.patch
trac_7666-reviewer.patch
trac_7835-preparsing-unicode_v2.patch
trac_7249_jinja2_v5.patch
trac_2779-sagenb-error-message.patch
2779_2_banner.patch
trac_3154-spurious-u0027-output.patch
trac_7969-escaped-backslash.patch
trac_7937-sass_manifest.patch
trac_4217-html-system-formatting.patch
trac_3083-print-documentation.patch
trac_7962-link-worksheets-zip-file.patch
trac_5177-delete-cell-dirs.patch

Sorry for the immense queue.

[kcrisman]

comment:4

I can't review this, but thank you so much for doing it - it really annoyed me and it's the sort of background thing that in the long run will make Sage so much better even if no one ever sees it.

[qed777]

comment:5

If I evaluate

print 'Hello!'
plot(sin(x))

then "Delete All Output," "Save & quit" and reopen the worksheet, the plot (not the text) reappears.

We could also delete the cells' files in Worksheet.delete_all_output. But we may wish to do this synchronously; otherwise, a long-running or simply delayed (e.g., on a busy server) thread might remove newly-written files.

(For deleting just one cell, there might be a similar but less likely race condition. For example, if after deleting a cell (with ID ) in the browser, a user pastes and saves {{{id=<id>|\n<updated code>///\n\n}}} in the "Edit" window, and re-evaluates the cell, a delayed Deferred could delete new files.)

What do most users expect/prefer?

I'm adding schilly to the Cc: list, since he's almost certainly better qualified than I on this (and many other) topics.

[qed777]

Attachment: trac_5177-delete-cell-dirs.3.patch.gz

Also delete files when deleting all output. Synchronous only. Replaces previous.

[qed777]

comment:6

V3 deletes the output synchronously (on the server), whether it's for one cell or for the whole worksheet.

I think this is much safer, since it avoids race conditions. Although deferred deletions could help performance, we already copy a cell's output synchronously. But with the appropriate locks or other synchronization constructs (e.g., "marking" cells for deletion), we could offload some tasks to other threads or to worksheet processes.

[qed777]

comment:7

We should be able to doctest both "hunks" on V3.

[qed777]

Attachment: trac_5177-delete-cell-dirs.4.patch.gz

Add doctects to V3. Apply only this patch. sagenb repo.

[qed777]

Changed author from Tim Dumol to Tim Dumol, Mitesh Patel

[TimDumol]

comment:9

Since a major problem with the notebook right now is its performance, and this may slow it down even further, I'm putting this to "Needs work; requires optimization".

[TimDumol]

Work Issues: Requires speed optimization

[jdemeyer]

Changed work issues from Requires speed optimization to none

[jdemeyer]

comment:10

Replying to @TimDumol:

Since a major problem with the notebook right now is its performance, and this may slow it down even further

How should this patch slow things down? The new code is only called when deleting a cell. In fact, it could even speed up the notebook as there will be fewer files around.

[jasongrout]

comment:11

Looks good to me. I applied a modified version to the flask notebook in this commit: http://code.google.com/r/jasongrout-flask-sagenb/source/detail?r=01b77dc4c1934eb0b54a16e19037f3d89f312482

[jasongrout]

comment:15

I'm not sure if I should have closed this. I'll leave it as positive review for now, since I suppose it could also be applied to the current notebook. The code looks fine to apply to the current notebook, but I don't know if it will apply cleanly, etc.

[jdemeyer]

comment:16

Replying to @jasongrout:

I'm not sure if I should have closed this.

Only the release manager should ever close tickets (with the exception of spam tickets or tickets which have no interesting content at all).

I'll leave it as positive review for now, since I suppose it could also be applied to the current notebook.

Yes, I will merge it.

This reminds me of the fact that we should really discuss how to merge the new notebook, and coordination between the current notebook and the new notebook. It's a topic which I started several times, but still I don't have a good answer.

[jdemeyer]

Reviewer: Jason Grout

[novoselt]

comment:18

Is this issue somehow related to #10234 by any chance?

[jasongrout]

comment:19

I fixed #10234 a while ago in the flask notebook. This patch is related; I rebased this patch around the (small) changes I made to fix #10234.

[jdemeyer]

comment:20

Replying to @jasongrout:

I fixed #10234 a while ago in the flask notebook. This patch is related

In which sense "related". Does this ticket also fix #10234? If you fixed #10234 already, it would be good also to merge #10234 in the current sagenb.

[jdemeyer]

Merged: sage-4.8.alpha3