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

Validate notebooks once per fetch or save #724

Merged
merged 3 commits into from
Mar 14, 2022

Conversation

kevin-bates
Copy link
Member

Some prior network analyses found that each fetch and save of a notebook file resulted in double validation, presumably so the server could set a helpful message on the notebook model returned to the user since the validation performed within nbformat's read or write methods did not return any indication of a validation error.

nbformat was recently updated to enable applications to ask for validation error status on the initial read/write calls, thereby enabling notebook to skip its second call to validate the notebook. This pull request leverages those updated signatures now available in nbformat 5.2.0.

The existing validate_notebook_model(), however, will still call nbformat.validate() if a None validation_error parameter is provided. This was done in order to preserve existing functionality from potential calls by server extensions. The message generation code is shared by both by raising the exception present in the validation_error dictionary.

(The CI failures do not appear to be related to these changes.)

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2022

Codecov Report

Merging #724 (34edc9d) into main (5d962d7) will increase coverage by 0.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #724      +/-   ##
==========================================
+ Coverage   70.95%   70.96%   +0.01%     
==========================================
  Files          62       62              
  Lines        7515     7523       +8     
  Branches     1190     1192       +2     
==========================================
+ Hits         5332     5339       +7     
- Misses       1826     1832       +6     
+ Partials      357      352       -5     
Impacted Files Coverage Δ
jupyter_server/services/contents/fileio.py 75.52% <80.00%> (ø)
jupyter_server/services/contents/filemanager.py 71.73% <100.00%> (+1.00%) ⬆️
jupyter_server/services/contents/manager.py 88.60% <100.00%> (+0.41%) ⬆️
jupyter_server/services/kernels/kernelmanager.py 80.69% <0.00%> (-1.90%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d962d7...34edc9d. Read the comment docs.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! Can you please install and run pre-commit on all the files?

pip install pre-commit
pre-commit install
pre-commit run --all-files

@kevin-bates
Copy link
Member Author

Thanks Steve, that was helpful.

@blink1073
Copy link
Contributor

Kicking CI to pick up fixes

@blink1073 blink1073 closed this Mar 14, 2022
@blink1073 blink1073 reopened this Mar 14, 2022
@blink1073 blink1073 merged commit 6e0e49e into jupyter-server:main Mar 14, 2022
@kevin-bates kevin-bates deleted the one-validate branch March 14, 2022 13:26
@kevin-bates kevin-bates mentioned this pull request Jun 24, 2022
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.

3 participants