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

Optionally speed up validation #1672

Merged
merged 1 commit into from Nov 14, 2021
Merged

Conversation

gwincr11
Copy link
Contributor

Motivation:

  • We are looking to build out a notebook viewer with an eye toward performance.
    In our flamegraph research we found that as much as 90% of the time spent in
    Notebook creation can occur in the preprocessor valdiation logic. This pr
    introduces the optional idea of optimistic validation, in which we care not
    about which preprocessor introduces a validation error but that the notebook
    ends in a valid state, since we care more about speed.

Related Issues:

Changes:

  • Introduces a config option to remove some of the validation checks.

Questions:

  • I am new to the project and am not sure where a test should live for
    this logic, or the best way to structure such a test. If a reference
    is available I will happily added a test.

Motivation:
  - We are looking to build out a notebook viewer with an eye toward performance.
    In our flamegraph research we found that as much as 90% of the time spent in
    Notebook creation can occur in the preprocessor valdiation logic. This pr
    introduces the optional idea of optimistic validation, in which we care not
    about which preprocessor introduces a validation error but that the notebook
    ends in a valid state, since we care more about speed.

Related Issues:
  - jupyter#1663

Changes:
  - Introduces a config option to remove some of the validation checks.

Questions:
  - I am new to the project and am not sure where a test should live for
    this logic, or the best way to structure such a test. If a reference
    is available I will happily added a test.
@gwincr11 gwincr11 changed the title Improve the speed of valdiation Optionally speed up valdiation Nov 12, 2021
Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

Thanks for making a PR for the change. There isn't a great example to copy for writing a test here. I'm good with merging as is but if you would like to add a test after you'd need to mock the validate call, trigger a preprocess event with the flag on, and then count the number of calls made.

@MSeal MSeal merged commit e6eb9f7 into jupyter:main Nov 14, 2021
@gwincr11
Copy link
Contributor Author

Thanks for the merge @MSeal I was less wondering how to test it and more wonder where it should live and how to setup the test properly, how to call nbconvert and pass it a file for example and what file this test best fits into. Thanks again!

@gwincr11 gwincr11 deleted the optimistic-validation branch November 30, 2021 15:53
@blink1073 blink1073 added this to the 6.4 milestone Dec 28, 2021
@blink1073 blink1073 changed the title Optionally speed up valdiation Optionally speed up validation Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants