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

add step to validate preprocessors that relies on nbformat 4.4 #645

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

mpacer
Copy link
Member

@mpacer mpacer commented Aug 10, 2017

This PR will break not pass on travis until jupyter/nbformat#103 is changed.

We had thought (in our discussion in #643) that preprocessors currently would not be able to violate the NotebookNode format… turns out, that isn't the case!

Lo, behold the PizzaPreprocessor:

class PizzaPreprocessor(Preprocessor):
    """Simple preprocessor that adds a 'pizza' entry to the NotebookNode.  Used 
    to test Exporter.
    """

    def preprocess(self, nb, resources):
        nb['pizza'] = 'cheese'
        return nb, resources

which is part of our TestExporter tests and is almost 4 years old.

This is definitely not a valid attribute, so it seems we've not been strict about enforcing the "nb→nb" aspect of Preprocessors for a while.

This change would enforce the constraint that @minrk suggested in #643, that we allow only additional attributes to be added to the intermediate representation of the notebook.

That would mean that the PizzaPreprocessor would now be ok (at least as far as the intermediate representation goes), as well as the cell.transient field used in #643.

It also would enforce this constraint on 3rd-party extensions relying on the native preprocessor support in the core Exporter (which is probably going to break some extensions but we probably want to know what extensions actually cause non-valid notebooks to be written).

@@ -308,4 +308,5 @@ def _preprocess(self, nb, resources):
#to each preprocessor
for preprocessor in self._preprocessors:
nbc, resc = preprocessor(nbc, resc)
nbformat.validate(nbc, relax_add_props=True)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth catching the validation error and re-raising with a message saying which preprocessor made it invalid?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is surprisingly non-trivial to do given the way that NotebookValidationError is written.

Copy link
Member Author

@mpacer mpacer Aug 15, 2017

Choose a reason for hiding this comment

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

see jupyter/nbformat#105 for more on why it's nontrivial.

Copy link
Member

Choose a reason for hiding this comment

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

I've posted the same idea on that nbformat issue, but for easy reference, this is what I'd suggest:

try: 
    nbformat.validate(nbc, relax_add_props=True)
except nbformat.ValidationError:
    self.log.error('Notebook is invalid after preprocessor %r', preprocessor)
    raise  # With no args, raise re-raises the same exception

It would be neater to have it in the exception message itself, but I think having it in logging is good enough.

@mpacer
Copy link
Member Author

mpacer commented Aug 17, 2017

Ok should be good to go now once nbformat is released

@mpacer mpacer modified the milestone: 5.3 Aug 18, 2017
@mpacer
Copy link
Member Author

mpacer commented Aug 21, 2017

Reran CI now that nbformat 4.4 is released and it passes. This should be good to merge.

@minrk minrk merged commit 2fedaf9 into jupyter:master Aug 21, 2017
@minrk minrk changed the title add step to validate preprocessors that relies on jupyter/nbformat#103 add step to validate preprocessors that relies on nbformat 4.4 Aug 21, 2017
@mpacer mpacer added unlogged and removed unlogged labels Aug 31, 2017
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