DOC: Remove outputs from example notebooks

[mgeier]

I think it sets a bad example when notebooks are committed with their outputs.

Also, when the outputs are committed, the notebooks are not executed (on RTD and during CI), therefore bugs are not noticed.

And that's the problem with this PR ... it exposes bugs in the notebooks, mostly related to changes in the templating system.

I don't know how to fix them, can somebody please help?

Feel free to ignore this PR and fix the problems in a separate PR.

[MSeal]

I generally agree that outputs shouldn't in the git source code, though some read-only views could present the notebooks with expected outcomes in documentation if they were used as such. I'm not aware of us doing that here so let's remove outputs as you have done.

Independent of having outputs or not, we can adopt this pattern from nbclient's test suite: https://github.com/jupyter/nbclient/blob/master/tox.ini#L61-L66 to test that all notebooks in the repo are runnable. We'd need to adjust it to search different paths for notebooks to run, but it's a good way to catch broken examples in PRs.

[MSeal]

For the template change issues in the example notebooks, I can look into that later in the week if no one else gets around to fixing the code there.

[mgeier]

Independent of having outputs or not, we can adopt this pattern from nbclient's test suite: https://github.com/jupyter/nbclient/blob/master/tox.ini#L61-L66 to test that all notebooks in the repo are runnable.

For notebooks that are part of the Sphinx docs, this is only needed if the notebooks have stored outputs. And even then, the nbsphinx_execute = 'always' option could theoretically be used (but I think it's much better just to remove those bulky outputs!).

The Sphinx build is already part of the CircleCI tests, any exceptions raised in notebooks will be detected there.

It would be even better to check for warnings as well, see #1295.

[mgeier]

@MSeal Did you have a chance to look at this?

[MSeal]

No sorry, I've been overwhelmed with the number of open threads. I'm dedicating some time this weekend to catching up, especially on nbconvert. I put this in my queue for the weekend.

[MSeal]

Been working on a backwards compatibility this afternoon patch to make the old examples work with a warning before updating them. I'll try to get it wrapped up tonight but it might be finished tomorrow.

[MSeal]

@SylvainCorlay @maartenbreddels In fixing the old examples I found a number of template loading issues. I updated this PR to add tests for those cases and makes old template assignment patterns backwards compatible with a deprecation warning. I had to do a fair bit of rework to the traitlet default patterns to fix everything, though I did have to leave one work-around to avoid infinite recursion in traitlet loads that could be dependent on each other. Logically speaking all the old cases work, except now if you don't provide any mimetype in your template it will default to the exporter mimetype instead of failing.

[mgeier]

Thanks for the update @MSeal!

There are still errors in CircleCI: https://app.circleci.com/pipelines/github/mgeier/nbconvert/46/workflows/4e2907c5-2797-404b-8ed0-b4d78511f548/jobs/46/steps

Those errors are not shown here because I guess they originally ran in by fork ... I don't know how to get them to show up here, any ideas?

In the meantime, I guess future test runs will be available here: https://app.circleci.com/pipelines/github/mgeier/nbconvert?branch=doc-clean-notebooks

[MSeal]

Hmm looks related to the circle CI setup for the docs tests. The environment isn't finding a python3 kernel. I'll look into it tomorrow more.

I tried tweaking some settings in CircleCI so hopefully it comments on the next change.

[MSeal]

Circle CI shows up on the PR now and adding the ipykernel dependency made it pass.

[MSeal]

@maartenbreddels @SylvainCorlay PR is passing, I'd like to get this merged and into the next alpha to make sure the backwards compatibility changes are functioning well if you have time to review.

[mgeier]

Now there is a warning left:

[IPKernelApp] WARNING | Config option `start` not recognized by `PelicanSubCell`.
[IPKernelApp] WARNING | Config option `end` not recognized by `PelicanSubCell`.  Did you mean `enabled`?

This is not a Sphinx warning, so it doesn't mark the CI run as "failing".

I guess this has something to do with traitlets and that Preprocessor cannot be used anymore as a traitlets-bearing base class?

[MSeal]

Hmm no that behavior shouldn't be impacted. Traitlets hasn't been touched in years. I think maybe the config is trying to load out of order. I'm debugging in now

[MSeal]

Sorry I've been busy, I think the example issue here not respecting flags in the example has been wrong for a while. I'll still dig into fixing it (in this PR if it's open, or in a later PR if not). Probably going to merge in another few days if no one else provides input on the PR.

[mgeier]

Thanks for the update @MSeal, sounds reasonable to merge before fixing the warning.

But at some point, we (I?) should probably try to detect those IPython warnings and make the Sphinx build error out.

Any ideas how to detect warnings when using nbconvert's ExecutePreprocessor?

[MSeal]

I think we should add an option to nbconvert to treat traitlet load warnings as errors so we could run it in a strict mode to catch these in the future.

[MSeal]

Uhg the conflicts are non-trivial to fix. I'll try to fix and merge tomorrow.

[maartenbreddels]

Difficult for me to review on mobile (vacation), the only thing I notice is the change of for loop order for root dir and template name. This would be different from voila, is there any strong reason for this change or could we revert that?

[SylvainCorlay]

There is a lot in there. I think that we should discuss this. Would you like to have a synchronous meering and do a review together?

[MSeal]

Yeah I'd be happy to do that. I've been a little slow on responding (work and life have gotten quite busy) but a video chat to talk through what was touched any why would be useful for everyone. I can prioritize getting feedback applied after that. I'm not sure what timezones folks are in, but I am in UTC-7 (US/Pacific) timezone and can make time early or late in the day most easily. @SylvainCorlay @maartenbreddels Is there a day / time that works for you all?

[MSeal]

@SylvainCorlay Do you have some time this weekend to chat on a call?

[SylvainCorlay]

@SylvainCorlay Do you have some time this weekend to chat on a call?

Can we chat on Sunday (evening EU time, morning for you?)

[MSeal]

Works for me. Shoot me an invite for a video call for a specific time that's convenient for you.

[MSeal]

Difficult for me to review on mobile (vacation), the only thing I notice is the change of for loop order for root dir and template name. This would be different from voila, is there any strong reason for this change or could we revert that?

If only the root_dirs is used then whenever a user supplies the template_path and the template_name together it will fail. The test_absolute_template_dir tests this case. To make old template paths to work and all the path configurations all work the code did need a lot of these changes :/ Since we already have had several alpha users hit issues where their old templates stopped working I felt it was important to ensure the compatibility, and ability to set the other attributes in combination.

Happy to dig through it tomorrow morning and figure out what to transition here.

Also I resolved the conflicts but something is amiss with the reveal template tests and I can't tell if it's related or not given the other parallel changes.

[maartenbreddels]

@MSeal is it important for compatibility that it should be template_paths that is assigned to?

[MSeal]

Unfortunately yes, but I think this was rarely directly used. I'd be ok with breaking compatibility on this field for simplicity and documenting to use the new field name if someone has an error. My understanding is that template_name was changed much more often and the working directory was used to set template_paths commonly. I might be wrong on that.

[maartenbreddels]

As discussed with @MSeal and @SylvainCorlay I had another pass at this. I basically reverted templateexporter.py to master, and made it such that the tests pass.
The approach here is that if template_name or template_file is an absolute file e.g.:
/some/path/acme/foo.tpl

it's split into template_file=foo.tpl, template_name=acme and /some/path/ is added to the extra_template_basedirs list trait, which is scanned for template_names in addition to the usually
$root_dir/nbconvert/templates/

This makes the code simpler to understand I think.

[SylvainCorlay]

it's split into template_file=foo.tpl, template_name=acme and /some/path/ is added to the extra_template_basedirs list trait, which is scanned for template_names in addition to the usually
$root_dir/nbconvert/templates/

Wow this is an elegant solution. Glad you thought of it.

I am all for merging the PR.

Then maybe we want to deprecate the user overriding template_name and advertize the new template system?

[maartenbreddels]

Yes, we have a warning now for template_name, not yet for template_file though. I'd also like to see these changes squashed before we merge this. I'll wait for @MSeal to take a look (maybe he can finish it), otherwise I'll finish/polish tomorrow.

[MSeal]

I agree that this is a much cleaner solution to the problem and the tests are all passing with this abstraction. Thanks for taking tackling this. I'd be good with a merge if you want.

[MSeal]

Ahh I missed that template_file needs a warning still in your comment

[MSeal]

I can get to that tomorrow if you want me to tackle that

[SylvainCorlay]

Let's get this in and get the warning in tomorrow.

[maartenbreddels]

I'd also like to see these changes squashed before we merge this.

now master is a big mess, do you mind (and can I?) force push to master to clean this up. I'd rather have history comprehensible, especially this part.

[MSeal]

Yes please do that, I have the full history local as well in case there's an issue from master cleanup

[MSeal]

I usually click the squash merge button on PRs when merging big commit lists (good default to use btw)

[MSeal]

After you fix the commit history I'll put branch protection on master.

[maartenbreddels]

It's now 3 commits, @mgeier original, the squash or all changes to the code, and the removal of example.html

[maartenbreddels]

My preference goes to never merge to master, is that possible? Only allow squash/rebase?

[MSeal]

Yep. I just turned off merge so only squash and rebase are available as options (rebase still adds each commit ontop). Also force pushes to master are disabled now

[SylvainCorlay]

Ok i can do that in an hour or so.

[maartenbreddels]

Master is good now, I already fixed it.

[mgeier]

I've created a new issue for a few of the remaining problems: #1339.

[MSeal]

Thanks @mgeier -- I'll take a look through that issue and see if we can triage some additional fixes / improvements this weekend.