IPEP 15: autosave the notebook #3158
Conversation
|
Okay, this actually autosaves now. The timer follows the following logic:
|
|
I think I get the logic behind firing every 10x the duration of the save (if it takes more than 3s), but feel it could do with a moments discussion... It might be good to put an upper limit on the time between saves - perhaps 5 minutes?? If for some bizarre reason, a particular save takes, say, 20s, then it'll be 3 mins 20secs before the next one. This can happen for example when the users client computer has a process that temporarily fills up it's memory, or perhaps the connection gets clogged by a flatmate downloading something huge or a cafe enforcing a rate limit. In such cases, it makes sense to reduce the load of autosaves on resources. However the flipside is that the user may be surprised that the notebook wasn't saved. The 20s -> 3mins20secs is perhaps ok, but what if a save takes a reallly long time, say 3 minutes? The situations in which I can imagine a save taking a long time:
|
|
It's a tricky choice. I would say that it's the right decision in an extremely low bandwidth case to have quite infrequent saves - even as infrequent as half an hour, if it really does take three minutes (most likely on a weak cellular connection or similar). The user is informed when the autosave frequency changes, so they will see "autosaving every 1800s" in the notification area if a save takes three minutes, and the 'last saved' area still has the last saved time stamp, etc. |
|
Yep, fair enough. And the user can always elect to manually save if they see that auto-saves are far apart. Does it detect how long the most recent save took? Or is it the most recent auto-save? My thinking is a situation where a save is unusually slow for a temporary reason - if the user does a manual save a little later that is quick, perhaps the autosave machinery should notice. As a matter of interest, is the notebook sent to the server as some sort of diff or as full text? I'm guessing full text. (I guess I'm thinking of google docs again - I believe they have a clever engine for that - I wonder if it can be borrowed?) |
|
Right now we send the entire notebook each time. In the future we plan on sending just the diffs, but that is going to take quite a bit more work. |
| @@ -104,7 +105,11 @@ | |||
| _kernel_id_regex = r"(?P<kernel_id>\w+-\w+-\w+-\w+-\w+)" | |||
| _kernel_action_regex = r"(?P<action>restart|interrupt)" | |||
| _notebook_id_regex = r"(?P<notebook_id>\w+-\w+-\w+-\w+-\w+)" | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Conflict markers got committed.
|
Done reviewing the server side of things. Overall the code is really clean and nice. I really like this design and its server side implementation a lot. Great work! Now onto the frontend side... |
|
The save button in the main toolbar still has a hover help text of "Save". This should be "Checkpoint" and maybe we should change the icon? |
|
Ahh, I see that "Save" still exists in the UI. I think this will confuse users. What do you think about only having "Checkpoint" be visible to the users. IOW, make save an internal detail...? |
|
Hmm, I am not seeing the behavior I was expecting in the UI. Not sure if it is broken or I just had false expectations of how it would work. |
|
|
||
| @line_magic | ||
| def autosave(self, arg_s): | ||
| """Set the lower-limit for the autosave frequency in the notebook (in seconds). |
There was a problem hiding this comment.
Do you mean upper limit here?
There was a problem hiding this comment.
I actually think we should say "Set the autosave interval in the notebook (in seconds)." here.
|
In case anyone really cares, we had an extensive discussion of this during a public Google hangout. We'll do more of those, open to anyone for participation, for issues that require high-bandwidth communication. |
|
Right now, the user invoked actions in the UI are for "Save and Checkpoint". But the "Last saved" date/time in the page header is for the last autosaved time. This is a bit confusing. In some cases, I think that users will want to see the time of last checkpoint instead. Thoughts? |
|
I can change it to the last checkpoint easily enough. Do we want to keep UI for the last autosave as well? |
|
Also, do we want a visual indication of the 'dirty' flag? |
|
While Google Docs uses a very different model of autosave than we have here, I feel like their messages about the save status are quite useful. You quickly see the save status of the document. I do think it would be useful to tell the user that there are unsaved changes, and tell them when the autosave has been done. That will put peoples mind at ease. Nto sure how to present all of that information though. |
| console.log("restore dialog, but no checkpoint to restore to!"); | ||
| return; | ||
| } | ||
| var dialog = $('<div/>').append( |
There was a problem hiding this comment.
I think I would make these different paragraphs a single one, or put spaces between them. Otherwise it just looks like poorly justified text.
instead of "please don't leave!"
autosave interval is tuned based on the duration of saves. Autosave will never happen more frequently than every 30 seconds, and if saves take more than 3 seconds, autosave will fire every 10x the duration of the save (i.e. if save takes 6 seconds, it will be every 60 seconds, etc.)
use IPythonHandler, avoid self.application
autosave is also there, but once the dirty flag is an event, this should stop having the date, and just become 'autosaved / unsaved changes'
also cleanup a few log messages
minor styling update to the restore dialog as well
|
My only concern at this point is purely cosmetic: with so much infromation printed about the checkpoint/save times, it doesn't take a very long notebook title before all that rolls over in a pretty ugly way: But improving this would really require rethinking the layout of the UI deeper. I think we'll need to do that, but I don't think it's wise to hold this important and complex PR any further just on that. Thoughts? |
|
It will be shorter when the dirty event is introduced, at which point the 'autosaved: DATE' will simply be |
|
Sounds good. Merging now, then! This was a long and painful one, kudos for a fantastic job, and I'm glad we suffered through the long and detailed discussion period. While in the end the chosen implementation isn't anything fancy or complex, I like that we took the time to dig at length into all the caveats and possible problems. Thanks!! |
Add autosaving to the notebook, while retaining manual checkpoint creation. Autosave will perodically save the notebook, with a default interval of two minutes. This interval is configurable, and the system will automatically back off the frequency if it detects that the save operation takes too long. This can be the case if working remotely over a slow link, so this will automatically conserve bandwidth. The manual save operation remains available and bound to the same keys, but now it causes the creation of a separate checkpoint, which is stored in a hidden directory called `.ipynb_checkpoints`. This provides users a poor-man's version control with a single revert point and the ability to safely experiment with potentially destructive changes to a notebook without concern that the autosave operation may clobber their file on disk. See [IPEP 15](https://github.com/ipython/ipython/wiki/IPEP-15%3A-Autosaving-the-IPython-Notebook) for details of the proposal.
Add autosaving to the notebook, while retaining manual checkpoint creation. Autosave will perodically save the notebook, with a default interval of two minutes. This interval is configurable, and the system will automatically back off the frequency if it detects that the save operation takes too long. This can be the case if working remotely over a slow link, so this will automatically conserve bandwidth. The manual save operation remains available and bound to the same keys, but now it causes the creation of a separate checkpoint, which is stored in a hidden directory called `.ipynb_checkpoints`. This provides users a poor-man's version control with a single revert point and the ability to safely experiment with potentially destructive changes to a notebook without concern that the autosave operation may clobber their file on disk. See [IPEP 15](https://github.com/ipython/ipython/wiki/IPEP-15%3A-Autosaving-the-IPython-Notebook) for details of the proposal.
See IPEP 15 for details of the proposal.
This is not quite complete, the most glaring omission is, of course, the autosave timer itself, but that's pretty trivial. The checkpointing machinery is fairly complete, and ready to discuss / review.
The gist: autosave will do a real save periodically. To address the undesirable effects of true autosave, user-initiated saves create checkpoints, which can be restored. In this preliminary implementation, the file-based notebook manager only supports one checkpoint.