Added a timeout argument to execute #48

AakashGfude · 2020-04-07T02:09:03Z

a timeout option to specify what is the maximum time allowed to execute for each cell before it times out. The default specified here is 30s, which is also the default value of ExecutePreProcessor.

chrisjsewell · 2020-04-07T07:05:23Z

As in https://nbsphinx.readthedocs.io/en/0.6.0/timeout.html#Cell-Execution-Timeout,
this should this also be controlled by notebook level metadata, i.e. there would be a global value (which for myst-nb could be an option in conf.py) which is used by default, but would be superseded by a value set in the notebook metadata, if present.

AakashGfude · 2020-04-07T07:14:41Z

Great. So, we will need to read the timeout metadata information somewhere in that loop before https://github.com/ExecutableBookProject/jupyter-cache/blob/517366e31adeafd4818fb40f7c85a364e44f600b/jupyter_cache/executors/basic.py#L159.

I will update this PR for this, and the one in myst-nb to check for conf variable

AakashGfude · 2020-04-08T11:42:12Z

Hey @chrisjsewell , a quick question. Where do you think the timeout keyword should reside here in the metadata. Like in the example above it is inside "nbsphinx" object.
We don't have any objects at present in metadata where timeout would fit. Nor I guess, it makes sense to create a new one unless we plan to have more variables.
Top-level sounds good enough?

chrisjsewell · 2020-04-08T17:21:05Z

Where do you think the timeout keyword should reside here in the metadata.

Not quite sure TBH, maybe execution/timeout rather than "polluting" the base namespace.
Would also be nice if it was something agreed on in nbclient

cc'in @choldgraf

choldgraf · 2020-04-10T01:27:17Z

I opened up jupyter/nbclient#45 to see if others in the nbclient repo have thoughts

AakashGfude · 2020-04-17T05:58:21Z

@choldgraf should we just create an object like nbsphinx does to store the timeout.

"mystnb": {
   "timeout": 60
}

No one seems to be replying to that thread. Probably there is no jupyter-standard, else nbsphinx might have used it as well?
Else we can wait a bit longer for their reply.

choldgraf · 2020-04-17T16:18:51Z

I think that you're right that there isn't really a standard, so let's just go with something that seems reasonable to us and we can support a new standard later on if we wish

chrisjsewell · 2020-04-17T16:50:11Z

jupyter-client doesn't/shouldn't have any direct ties to mystnb though, so it would be better to use a more generic top level key like execution or nbclient which we would then suggest in jupyter/nbclient#45

AakashGfude · 2020-04-20T00:26:17Z

jupyter-client doesn't/shouldn't have any direct ties to mystnb though, so it would be better to use a more generic top level key like execution or nbclient which we would then suggest in jupyter/nbclient#45

ok, so committing again with execution key then

Added

"execution": {
     "timeout":  
}

choldgraf · 2020-04-20T01:06:23Z

Just a note that matt did have some thoughts in jupyter/nbclient#45 he just posted...it mostly comes down to "there is no current standard" but I think nbclient folks are open to agreeing on a metadata spec that is reasonable.

AakashGfude · 2020-04-20T01:11:11Z

@choldgraf that's great. So, basically he prefers to have separate metadata for execution as well instead of namespaced subsection. Then we can ditch the option of putting timeout in nbclient and can use execution instead in the top-level?

choldgraf · 2020-04-20T01:25:54Z

I'm not entirely sure - but feel free to throw out some ideas in the nbclient thread and perhaps we can come to a quick agreement

choldgraf · 2020-05-22T17:59:17Z

@AakashGfude this looks good from a functionality standpoint, three major points:

Could you document the timeout parameter in the docstring of one of those classes? E.g. make it clear that the number is in seconds, and also add to the docs of jupyter-cache about how to control this?
Could you add a test to make sure that this works as expected. Maybe just add the notebook timeout metadata to one of the test notebooks and make sure there's a computation in there that both: 1. breaks the timeout, and 2. happens underneath the timeout
I think it should also be possible to control the timeout globally from, say, the command line or the Python API. That way downstream in Jupyter Book, we could set a single config like execution_timeout: 60 and it would then set the timeout for every notebook execution, rather than requiring us to put the metadata in every notebook.

Does that all make sense? Lemme know what you think.

AakashGfude · 2020-05-23T15:45:58Z

@choldgraf have added tests and also added this as an option in cli commands, coz it might come handy?
usage example (for 10 second timeout):-

jb execute --timeout 10 or jb execute -t 10

Could you document the timeout parameter in the docstring of one of those classes? E.g. make it clear that the number is in seconds, and also add to the docs of jupyter-cache about how to control this?

have added a line in python API section of the docs. But, the execute command line option has none of its parameters specified in the docs, so, was not sure where to put it.
would like to know your thoughts.

Could you add a test to make sure that this works as expected. Maybe just add the notebook timeout metadata to one of the test notebooks and make sure there's a computation in there that both: 1. breaks the timeout, and 2. happens underneath the timeout

Thanks. Have added the tests now.

I think it should also be possible to control the timeout globally from, say, the command line or the Python API. That way downstream in Jupyter Book, we could set a single config like execution_timeout: 60 and it would then set the timeout for every notebook execution, rather than requiring us to put the metadata in every notebook.

it indeed was setup to handle a global config value for timeout , In jupyter-book inside the execute object in _config.yml, we can have a timeout key.

AakashGfude · 2020-05-23T16:13:04Z

This is so weird. Not sure, what got updated, but pre-commit tests are failing for files in Travis in which I did not work on. Needless to say, Working fine on my local system.

AakashGfude · 2020-05-23T16:29:12Z

not sure why complex_outputs*.ipynb files are failing in Travis CI. Probably because of lack of installed packages in the server. I will have a deeper look later.

AakashGfude · 2020-05-24T13:48:05Z

Have added the same files complex_outputs_unrun and complex_outputs_unrun_metadata as MyST-NB tests. and also the same extra testing dependencies.

choldgraf · 2020-05-24T14:25:17Z

nice! is this ready for a review or merge?

AakashGfude · 2020-05-25T00:26:42Z

@choldgraf yes

choldgraf

This looks good to me - just two quick comments in there 👍 thanks Aakash!

jupyter_cache/cli/commands/cmd_cache.py

choldgraf · 2020-05-27T15:22:27Z

jupyter_cache/executors/basic.py

@@ -57,12 +57,13 @@ class JupyterExecutorBasic(JupyterExecutorAbstract):

    The execution is split into two methods: `run` and `execute`.
    In this way access to the cache can be synchronous, but the execution can be
-    multi/async processed.
+    multi/async processed. Takes timeout parameter in seconds for execution


Could you add a short section on "timeout" under the execution documentation here: https://jupyter-cache.readthedocs.io/en/latest/using/api.html#execution

And mention that you can specify a timeout either via a the call to jupyter-cache, or via the execution: timeout: metadata field?

I had added a small Note there https://github.com/executablebooks/jupyter-cache/pull/48/files#diff-7d5c2179cfd8d418561a37c077b98278R1063 . But, creating a small section sounds better

@choldgraf what do you think?

choldgraf · 2020-05-29T23:20:53Z

ok this LGTM and the integration tests are passing, so I'll give it a merge!

choldgraf · 2020-05-29T23:21:06Z

thanks very much @AakashGfude for working on this one! 👍

AakashGfude added 2 commits April 7, 2020 11:50

added a timeout argument to execute

004def3

adding 30s instead of None

517366e

AakashGfude mentioned this pull request Apr 7, 2020

adding a timeout for execution executablebooks/MyST-NB#140

Merged

choldgraf mentioned this pull request Apr 10, 2020

A standard for execution config metadata? jupyter/nbclient#45

Open

checking execution metadata key

007f52b

AakashGfude added 2 commits May 24, 2020 01:03

added tests and command line paramter

ffbdd73

added a line in doc

c2b2def

AakashGfude force-pushed the exec-timeout branch 2 times, most recently from dc84e82 to d4a6bbc Compare May 23, 2020 15:45

AakashGfude force-pushed the exec-timeout branch 3 times, most recently from 8b5df22 to a7c2b8d Compare May 23, 2020 16:01

AakashGfude force-pushed the exec-timeout branch from a7c2b8d to 1af60c3 Compare May 23, 2020 16:18

default in number

0326dfa

AakashGfude force-pushed the exec-timeout branch from 1af60c3 to 0326dfa Compare May 23, 2020 16:20

added noqa's for sudden pre-commit warnings

7528b94

added test package de[endency

557e184

AakashGfude force-pushed the exec-timeout branch from 2884fc6 to 557e184 Compare May 24, 2020 13:40

choldgraf reviewed May 27, 2020

View reviewed changes

added documentation and removed F541 pre-commit checks

1d7f01f

choldgraf merged commit 55db432 into executablebooks:master May 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a timeout argument to execute #48

Added a timeout argument to execute #48

AakashGfude commented Apr 7, 2020

chrisjsewell commented Apr 7, 2020

AakashGfude commented Apr 7, 2020

AakashGfude commented Apr 8, 2020

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 10, 2020

AakashGfude commented Apr 17, 2020 •

edited

Loading

choldgraf commented Apr 17, 2020

chrisjsewell commented Apr 17, 2020 •

edited

Loading

AakashGfude commented Apr 20, 2020 •

edited

Loading

choldgraf commented Apr 20, 2020

AakashGfude commented Apr 20, 2020

choldgraf commented Apr 20, 2020

choldgraf commented May 22, 2020 •

edited

Loading

AakashGfude commented May 23, 2020 •

edited

Loading

AakashGfude commented May 23, 2020 •

edited

Loading

AakashGfude commented May 23, 2020

AakashGfude commented May 24, 2020 •

edited

Loading

choldgraf commented May 24, 2020

AakashGfude commented May 25, 2020

choldgraf left a comment

choldgraf May 27, 2020

AakashGfude May 28, 2020

AakashGfude May 28, 2020

choldgraf commented May 29, 2020

choldgraf commented May 29, 2020

Added a timeout argument to execute #48

Added a timeout argument to execute #48

Conversation

AakashGfude commented Apr 7, 2020

chrisjsewell commented Apr 7, 2020

AakashGfude commented Apr 7, 2020

AakashGfude commented Apr 8, 2020

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 10, 2020

AakashGfude commented Apr 17, 2020 • edited Loading

choldgraf commented Apr 17, 2020

chrisjsewell commented Apr 17, 2020 • edited Loading

AakashGfude commented Apr 20, 2020 • edited Loading

choldgraf commented Apr 20, 2020

AakashGfude commented Apr 20, 2020

choldgraf commented Apr 20, 2020

choldgraf commented May 22, 2020 • edited Loading

AakashGfude commented May 23, 2020 • edited Loading

AakashGfude commented May 23, 2020 • edited Loading

AakashGfude commented May 23, 2020

AakashGfude commented May 24, 2020 • edited Loading

choldgraf commented May 24, 2020

AakashGfude commented May 25, 2020

choldgraf left a comment

Choose a reason for hiding this comment

choldgraf May 27, 2020

Choose a reason for hiding this comment

AakashGfude May 28, 2020

Choose a reason for hiding this comment

AakashGfude May 28, 2020

Choose a reason for hiding this comment

choldgraf commented May 29, 2020

choldgraf commented May 29, 2020

AakashGfude commented Apr 17, 2020 •

edited

Loading

chrisjsewell commented Apr 17, 2020 •

edited

Loading

AakashGfude commented Apr 20, 2020 •

edited

Loading

choldgraf commented May 22, 2020 •

edited

Loading

AakashGfude commented May 23, 2020 •

edited

Loading

AakashGfude commented May 23, 2020 •

edited

Loading

AakashGfude commented May 24, 2020 •

edited

Loading