Increase timeout for live docs #147

Merged
merged 8 commits into from Apr 2, 2013

Projects

None yet

4 participants

@kini
Sage Mathematical Software System member

Note: it seems that this setting 'idle_timeout' is only used for live
documentation browser pages, so it should be safe to change it without
affecting users' worksheets' behavior.

@nthiery nthiery Increase timeout for live docs
Note: it seems that this setting 'idle_timeout' is only used for live
documentation browser pages, so it should be safe to change it without
affecting users' worksheets' behavior.
5993b5c
@ppurka
Sage Mathematical Software System member

This parameter is also used for timing out normal worksheets. So, it is not correct to write it as timeout of the live documentation only.

@kini
Sage Mathematical Software System member

Are you sure? The only place I see it being used is here: https://github.com/sagemath/sagenb/blob/master/sagenb/notebook/notebook.py#L1252

Note the if W.docbrowser().

@kini
Sage Mathematical Software System member

That is reading it, not setting it.

@ppurka
Sage Mathematical Software System member

Well, you can set it too. I don't remember where. I do use that parameter (i.e. timeout) on banana. Will let you know when I find out :)

@kini
Sage Mathematical Software System member

Certainly, you can set it, but if it is not read anywhere except the line I mentioned -- which appears to be the case -- then it is not used to time out anything except docbrowser worksheets, i.e. the live documentation, no matter what it is set to.

By the way, you can set it by passing it as a parameter to the notebook() call or to sage -n.

@ppurka
Sage Mathematical Software System member
@kini
Sage Mathematical Software System member

What's misleading about it? Is it used for anything else other than live documentation?

@ppurka
Sage Mathematical Software System member

It is set from the timeout parameter. Also look at the function here
https://github.com/sagemath/sagenb/blob/master/sagenb/notebook/notebook.py#L1251
The idle_timeout parameter does affect both documentation and normal worksheets.

@kini
Sage Mathematical Software System member
@ppurka
Sage Mathematical Software System member

Yes, but that happens only if timeout is zero. Otherwise all open worksheets are quit as long as the timeout is nonzero. The nonzero value is obtained from idle_timeout.

@kini
Sage Mathematical Software System member

D'oh, you're right, sorry :)

kini added some commits Apr 2, 2013
@kini kini Make separate config var for pub timeout
Note: the commit message of the parent of this commit is incorrect. The
timeout variable did control all worksheets, hence this commit.
92ac384
@kini kini Rearrange config variable descriptions 88abcf3
@kini
Sage Mathematical Software System member

@ppurka or @nthiery, can you review this?

@nthiery

I don't know enough the notebook code to really review, but this sounds reasonable.

That being said, I'd increase the default timeout for live worksheets to 10 minutes (then you have most likely switched to some other task, while spending two minutes thinking or scratching some paper is usual).

Thanks for your work on that issue!

@ppurka
Sage Mathematical Software System member

I think this needs a more careful fix, now that you have separated the timeouts.

  1. Can you put some comments beside the timeouts explaining what they are for? Is idle only for worksheets? And what is pub for?

  2. Depending on which timeout is for worksheets, we should change the line on https://github.com/sagemath/sagenb/blob/master/sagenb/notebook/run_notebook.py#L537 to the user specified timeout.

@ppurka
Sage Mathematical Software System member

Oops. Sorry, that was so silly of me. The dicts contain the descriptions.

@nthiery

And for the record, I should mention here my original motivation: my
main use of live documentation is to go through sage tutorials, course
notes and the like during talks and classes. See e.g.:

http://combinat.sagemath.org/doc/thematic_tutorials/talks.html

It's very anoying to type in some stuff, pause for two minutes to
explain something, and come back to discover that all the previously
set variables were silently reset.

Which points to an independent thing: there should be some visual
feedback when there is a reset. Should I create a separate issue for
that?

Of course this will be a less crucial feature when it will be possible
to import rst documents.

@kini
Sage Mathematical Software System member

@nthiery, for now, can you just set the timeout yourself in the notebook settings? For example on sagenb.org if we suddenly increase the timeout by a factor of 5, the server load may jump significantly. What we're editing in this pull request is just the default values.

Alternatively, if you or ppurka wants to give this positive review, I can put it into sagenb 0.10.6 and immediately put it on the Sage trac. (I screwed up 0.10.5, which now doesn't even load into Sage, haha.)

Which points to an independent thing: there should be some visual
feedback when there is a reset. Should I create a separate issue for
that?

Please do, but I'm afraid I'm not familiar enough with the notebook to actually implement that...

@ppurka ppurka and 1 other commented on an outdated diff Apr 2, 2013
sagenb/notebook/notebook.py
for W in self.__worksheets.values():
if W.compute_process_has_been_started():
- W.quit_if_idle(timeout)
+ if W.docbrowser():
+ W.quit_if_idle(pub_timeout)
@ppurka
ppurka Apr 2, 2013

This should be doc_timeout, as well as the line 1253 before it. There is no need for a pub_timeout since published worksheets are noninteractive.

@kini
kini Apr 2, 2013

Good point... I somehow thought that published worksheets could be interactive. Fixed in 7346a9a.

kini and others added some commits Apr 2, 2013
@kini kini pub_timeout is useless
Thanks to @ppurka for pointing out that published worksheets are never
interactive anyway.
7346a9a
@ppurka ppurka add options for live documentation timeout to the notebook command 2f3aaa9
@ppurka ppurka commented on the diff Apr 2, 2013
sagenb/notebook/server_conf.py
@@ -75,7 +69,15 @@
},
'idle_timeout': {
- DESC : _('Idle timeout (seconds)'),
+ POS : 1,
+ DESC : _('Idle timeout for normal worksheets (seconds)'),
+ GROUP : G_SERVER,
+ TYPE : T_INTEGER,
+ },
+
+ 'doc_timeout': {
+ POS : 3,
@ppurka
ppurka Apr 2, 2013

What is this POS? Shouldn't it be 2 now?

@kini
kini Apr 2, 2013

Indeed, haha. I had POS : 2 for pub_timeout before we decided that was useless. In practice it doesn't matter, though - these POS variables are just used for ordering and most of the config entries don't even have a POS set (which means it defaults to 100).

@ppurka ppurka and 1 other commented on an outdated diff Apr 2, 2013
sagenb/notebook/server_conf.py
@@ -13,14 +13,14 @@
defaults = {'word_wrap_cols':72,
'max_history_length':250,
-
- 'idle_timeout':120, # 2 minutes
+
+ 'idle_timeout': 120,
+ 'doc_timeout': 120,
@ppurka
ppurka Apr 2, 2013

So, you are fairly certain that setting doc_timeout to 600 won't cause any "harm"? :) In that case, we can change it to 600. Let me pull in @jasongrout to find out what his opinion is w.r.t. this change.

@jasongrout
jasongrout Apr 2, 2013

Sure, I guess. I thought we increased the timeout a long time ago to avoid the infamous text cell corruption bug. I think the default timeouts should be much longer than 2 minutes, but I haven't looked at the code in a while, so I'm not absolutely certain of the ramifications.

@ppurka
ppurka Apr 2, 2013

Thanks. I have changed it to 600 in the other pull request: kini#1

ppurka and others added some commits Apr 2, 2013
@ppurka ppurka increase timeout for live documents to 600s 9a0b7a4
@kini kini Merge pull request #1 from ppurka/livedoc_timeout
add options for live documentation timeout to the notebook command
ffe48b4
@ppurka
Sage Mathematical Software System member

Ok. I have no more problems with this patch. I believe it is good to go.

@kini
Sage Mathematical Software System member

Cool, thanks :)

@kini kini merged commit 4b0aeb6 into sagemath:master Apr 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment