Make execute preprocessor traits configurable #1690
Conversation
|
|
|
Thanks @tmetzl. Could you also add help messages ? |
|
I copied the original help messages from nbclient |
|
Thinking again about this, I'm not sure about these changes. Some of these parameters need to be set:
|
|
This PR is not changing any of the default values. It just makes them configurable. I don't see harm in letting the user override the sensible default values picked here. What I really need is the ability to configure the timeout. Currently I have to use my own ExecutePreprocessor since otherwise everything times out in 30s which is not enough time to autograde my students. |
|
I agree that the timeout should be configurable. I remember why it was implemented the first time. It is necessary when the cell contains code to redirect the traceback (eg. https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb). In this specific case, the execution never ended, and the timeout is necessary. Perhaps we could add a minimal value > 0 and do not allow none value to guarantee a positive timeout. |
The nbgrader project is aware of a form of cheating where students disrupt `InteractiveShell.showtraceback` in hopes of hiding exceptions to avoid losing points. They have implemented a solution to prevent this cheating from working on the client side, and have some tests to demonstrate this technique: https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt-alternative.ipynb In essence, these attacks import the interactive shell and erase the traceback handler so that their failing tests won't report failures. import IPython.core.interactiveshell IPython.core.interactiveshell.InteractiveShell.showtraceback = None The problem is that this causes an exception inside the kernel, leading to a stalled execution. The kernel has stopped working, but the client continues to wait for messages. So far, nbgrader's solution to this is to require a timeout value so the client can eventually decide it is done. This prevents allowing a value of `None` for `Execute.timeout` because this would cause a test case to infinitely hang. This commit addresses the problem by making `InteractiveShell._run_cell` a little more protective around it's call to `showtraceback()`. There is already a try/except block around running the cell. This commit adds a finally clause so that the method will _always_ return an `ExecutionResult`, even if a new exception is thrown within the except clause. For the record, the exception thrown is: TypeError: 'NoneType' object is not callable Accepting this change will allow nbgrader to update `nbgrader.preprocessors.Execute` to support a type of `Integer(allow_none=True)` as the parent `NotebookClient` intended. Discussion about this is ongoing in jupyter/nbgrader#1690.
I was looking into why this is the case, and I realized it doesn't have to be. This attack causes an unhandled exception in the kernel, which is why execution hangs indefinitely. I just issued a PR upstream in iPython to handle (read: ignore) that exception so we can still return a result. I tested this alongside allowing I have no idea how long that will take to merge upstream, but once it does, I think we're safe on that front. The upstream |
nbgrader/preprocessors/execute.py
Outdated
| def on_cell_executed(self, **kwargs): | ||
| @validate('timeout') | ||
| def _validate_timeout(self, proposal): | ||
| value = int(proposal['value']) |
There was a problem hiding this comment.
This current validation throws an exception when the value is None.
nbgrader/preprocessors/execute.py
Outdated
| """ | ||
| The time to wait (in seconds) for output from executions. | ||
| If a cell execution takes longer, a TimeoutError is raised. | ||
| ``None`` or ``-1`` will disable the timeout. If ``timeout_func`` is set, |
There was a problem hiding this comment.
It would be my preference to only support None and positive integers. If that's the case, this docstring needs to be updated. If not, more code changes are needed.
|
Thanks for your work on this so far, @tmetzl ! |
The nbgrader project is aware of a form of cheating where students disrupt `InteractiveShell.showtraceback` in hopes of hiding exceptions to avoid losing points. They have implemented a solution to prevent this cheating from working on the client side, and have some tests to demonstrate this technique: https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt-alternative.ipynb In essence, these attacks import the interactive shell and erase the traceback handler so that their failing tests won't report failures. ```python import IPython.core.interactiveshell IPython.core.interactiveshell.InteractiveShell.showtraceback = None ``` The problem is that this causes an exception inside the kernel, leading to a stalled execution. The kernel has stopped working, but the client continues to wait for messages. So far, nbgrader's solution to this is to require a timeout value so the client can eventually decide it is done. This prevents allowing a value of `None` for `Execute.timeout` because this would cause a test case to infinitely hang. This commit addresses the problem by making `InteractiveShell._run_cell` a little more protective around it's call to `showtraceback()`. There is already a try/except block around running the cell. This commit adds a finally clause so that the method will _always_ return an `ExecutionResult`, even if a new exception is thrown within the except clause. For the record, the exception thrown is: TypeError: 'NoneType' object is not callable Accepting this change will allow nbgrader to update `nbgrader.preprocessors.Execute` to support a type of `Integer(allow_none=True)` as the parent `NotebookClient` intended. Discussion about this is ongoing in jupyter/nbgrader#1690.
|
Thanks @shreve, it works well with
I'm still confused about the value of being able to set |
|
Once you decided how to move forward please let me know what I need to change to make it work. |
I think from an end-user perspective it makes sense. If you have an assignment where each test must pass before the next one should be attempted, why waste the resources running cells that you know will fail? Do you have more detail about what systems/functionality will fail in that scenario? I'm not incredibly familiar with the whole codebase yet. |
This would break the rendering of the validation dialog. I think it deserves more discussion since it breaks a part of the rendering of |
Update the traits on the Execute preprocessor based on the discussion in jupyter#1690, including adding a version pin of ipython >= 8.10.0
Update the traits on the Execute preprocessor based on the discussion in jupyter#1690, including adding a version pin of ipython >= 8.10.0 Also, reorder traits to match the upstream parent, NotebookClient.
Update the traits on the Execute preprocessor based on the discussion in jupyter#1690, including adding a version pin of ipython >= 8.10.0 Also, reorder traits to match the upstream parent, NotebookClient.
@brichet I agree. The change to allow_errors is more influential than I thought. This would be great before a 1.0, but deserves it's own ticket. @tmetzl I made a PR against yours with the changes I think we need here. I was expecting to be able to remove the |
|
Oof, ok. iPython dropped support for python 3.7 a little over a year ago, so setting the minimum version broke nbgrader's compatibility. In that commit, they cite NEP 29 — Recommend Python and NumPy version support as a community policy standard which suggests a time table for dropping support for older pythons which NumPy recommends to the rest of the community as well. In order to merge this change, we now need to figure out a strategy for dealing with python 3.7 and implement it. I would personally be in favor of upping the minimum from 3.7 to 3.8, but we could pick another way forward. @brichet what would your preference be? Does a decision like this need wider approval? |
|
Thanks @shreve for changes and @tmetzl for merging it.
I don't have a strong opinion on dropping unmaintained Python version.
I think it can be removed, since it was introduced to avoid infinite loop in case of a cheat attempt using |
|
Great! I see that change was merged. I think that means with a rebase off main, this PR should be good to go! |
|
@tmetzl Are you able to do this update? Either rebase off main or merge main in. It would be awesome to get this merged in time for 0.8.2. |
Update the traits on the Execute preprocessor based on the discussion in jupyter#1690, including adding a version pin of ipython >= 8.10.0 Also, reorder traits to match the upstream parent, NotebookClient.
|
Beautiful! Thanks all! |
The following traits are not configurable:
This means you can't adjust the timeout via
nbgrader autograde <My_Assignment> --Execute.timeout=120. This will throw an error"[AutogradeApp | WARNING] Config option `timeout` not recognized by `Execute`. Did you mean one of: `iopub_timeout, startup_timeout, timeout_func`?"By making the traits configurable the command line or config file option is used correctly.