Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeout configuration in validation and error message #1730

Merged
merged 3 commits into from Jun 15, 2023

Conversation

tuncbkose
Copy link
Contributor

@tuncbkose tuncbkose commented Jan 31, 2023

Make timeout configuration work via:

  • either c.Execute.timeout or c.ExecutePreprocessor.timeout in nbgrader_config.py or,
  • --Execute.timeout or --ExecutePreprocessor.timeout in the command-line.

Since #1690 is now merged, this pr also makes error_on_timeout configurable and makes it actually printed on command line or included in Notebook/Lab response messages.

In the case of validating, the current implementation does not use configuration in the way traitlets expects, in that it just calls preprocessor({"timeout": x}), which is caught by ExecutePreprocessor (this may work, but only by accident). By giving the argument parent=self, the configuration is deferred to the initialization of Configurable, which knows how to do it properly.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch AaltoSciComp/nbgrader/timeout

@tuncbkose tuncbkose force-pushed the timeout branch 2 times, most recently from b5e50f9 to c3176fb Compare February 1, 2023 09:20
@tuncbkose
Copy link
Contributor Author

I see that #1690 implements timeout configuration like this, but better. The validate part of this pr is still worth considering in my opinion. I can force-push to keep only that part if requested

@tuncbkose
Copy link
Contributor Author

The commit above makes error_on_timeout configurable and makes it actually printed on command line or included in Notebook/Lab response messages.

@tuncbkose
Copy link
Contributor Author

Different behavior on Windows? I can try to get a machine to take a closer look, but that might not happen soon.

@tuncbkose
Copy link
Contributor Author

I don't exactly understand the details, but the Windows errors were seemingly caused by timeout being set too short. Setting it to 2 seconds is fine, but when it is only 1, my guess is that the kernel gets cancelled too fast for nbclient to handle. I'll try to investigate more and make an issue for nbclient if appropriate.

@tuncbkose tuncbkose changed the title Timeout configuration Timeout configuration in validation and error message Apr 6, 2023
Copy link
Contributor

@brichet brichet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tuncbkose, LGTM.
I just have a comment below.

nbgrader/preprocessors/execute.py Show resolved Hide resolved
@tuncbkose tuncbkose force-pushed the timeout branch 2 times, most recently from 4ef9447 to 6ebc18b Compare June 13, 2023 09:26
@tuncbkose
Copy link
Contributor Author

tuncbkose commented Jun 13, 2023

The nbextensions failures seem to be related to the new selenium version deprecating service_log_path in nbgrader/tests/nbextensions/conftest.py::_make_browser
Edit: created #1789 to fix this

@tuncbkose
Copy link
Contributor Author

Rebased for testing fixes

Copy link
Contributor

@brichet brichet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

I only have some prettier suggestions.

nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
nbgrader/tests/apps/test_nbgrader_validate.py Outdated Show resolved Hide resolved
@tuncbkose
Copy link
Contributor Author

Thank you! Will amend these suggestions in a bit.

@brichet brichet merged commit 129fe10 into jupyter:main Jun 15, 2023
25 checks passed
@tuncbkose tuncbkose deleted the timeout branch June 15, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants