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

VoilaExecutePreprocessor.allow_errors always True #519

Open
jeffyjefflabs opened this issue Jan 17, 2020 · 12 comments
Open

VoilaExecutePreprocessor.allow_errors always True #519

jeffyjefflabs opened this issue Jan 17, 2020 · 12 comments

Comments

@jeffyjefflabs
Copy link
Contributor

If a cell throws an exception, the cells after it are still being executed. Setting allow_errors explicitly to False (which is the default anyway) doesn't affect it.

@jtpio
Copy link
Member

jtpio commented Jan 24, 2020

Good catch, thanks @jeffyjefflabs.

Voila handles CellExecutionError here:

except CellExecutionError as e:

Which seems to be shadowing the allow_errors flag from nbconvert.

@martinRenou
Copy link
Member

I am working on this today

@martinRenou
Copy link
Member

It comes from #186, I feel like this PR should not have been merged as it shadows the CellExecutionError generated by nbconvert when allow_errors is set to False (see https://github.com/jupyter/nbconvert/blob/master/nbconvert/preprocessors/execute.py#L477)

@martinRenou
Copy link
Member

Although I agree with the initial intention of this PR. Maybe we should set allow_errors to True by default in Voila?

@maartenbreddels
Copy link
Member

I agree, since this looks to be controllable from nbconvert, voila should not add similar behavior. Maybe allow_error should be set to True on debug?

@martinRenou
Copy link
Member

martinRenou commented Jan 28, 2020

What I have in mind is to overwrite the allow_errors default value to True in Voila, so that Voila does not show a 500 error by default if the execution failed.

And in the case of having allow_errors to True (default), errors are only shown in debug mode.

@maartenbreddels
Copy link
Member

Sound good I think, maybe we can use this pattern:

def default_config(self):

@martinRenou
Copy link
Member

martinRenou commented Jan 28, 2020

Completely overwriting the trait would allow writing the right documentation in voila --help-all:

allow_errors = Bool(
    True,
    help=dedent(
        """
        If `True` (default), execution errors are ignored and the execution
        is continued until the end of the notebook. Output from
        exceptions is included in the cell output only if Voila is run with --debug.
        If `False`, when a cell raises an error the
        execution is stopped and a `CellExecutionError`
        is raised.
        """
    )
).tag(config=True)

@maartenbreddels
Copy link
Member

I remember this being an issue actually, not 100% sure it is related: jupyter/nbconvert#1056 (comment)

@martinRenou
Copy link
Member

I am not sure I understand the comment on your link, what was the issue?

@maartenbreddels
Copy link
Member

If you add --allow-errors to aliases (like in jupyter/nbconvert@e78f02e) traitlets gets confused which class to alias this to. But as long as we don't make it an alias we should be fine.

@jeffyjefflabs
Copy link
Contributor Author

Thanks for looking into this! I tried your PR #530 and it works for me. I like keeping allow_errors=False by default; for most of our use cases we definitely don't want to try to keep going after an error.

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 a pull request may close this issue.

4 participants