Add kernel-specific templates

[flying-sheep]

this adds kernel-specific templates and removes caching. several runs of the test suite showed that caching had no effect on its runtime that is above noise level.

[takluyver]

Kernel specific templates is something that probably requires more discussion, and I'd be happier if that was separate from the purely mechanistic change of removing caching.

It's probably fine to get rid of caching; the caching design itself was a rationalisation of a buggy stateful API. However, our test suite is unlikely to be a good performance test. In particular, if you're always initialising a new Exporter, there's self-evidently no benefit to caching. So I'd like to see some comparisons with an exporter initialised once and then reused many times. If we have to read the template file again each time, it may be meaningful to keep the cache.

Jinja appears to have its own cache of templates, but that's on the environment, so if we recreate the environment each time as well, we can't use that.

[flying-sheep]

we could just cache the environments.

and tbh, i mainly removed the caching because i couldn’t get it to work with per-kernel templates. something really weird goes on there

[flying-sheep]

uuh, this test failure is really weird, can anyone shed light on it?

[takluyver]

I've prodded it to rerun in case it was a sporadic failure.

[takluyver]

And it seems it was.

[flying-sheep]

oo, how to rerun without closing and reopening again?

[takluyver]

Travis has a button to do it, but it's only available if you have push access to the repo.

[flying-sheep]

OK, wtf?

it says it uses the correct template, but the wrong stuff is put out?

or it uses the wrong template, but for some reason get_source finds the correct one.

in any case i can’t debug it since it works locally.

[takluyver]

That appears to me to be saying it's loading the default template, not your custom one:

Loaded template markdown.tpl from .../nbconvert/exporters/../templates/markdown.tpl

[takluyver]

https://docs.python.org/3/library/test.html

The test package is meant for internal use by Python only. It is documented for the benefit of the core developers of Python. Any use of this package outside of Python’s standard library is discouraged as code mentioned here can change or be removed without notice between releases of Python.

We already require testpath for tests, so you can use testpath.modified_env to achieve this.

[flying-sheep]

thanks, will do!

[flying-sheep]

oh, true. but why? here i add the kernel path right after the custom path, so the loader should try (and find) stuff there first.

[takluyver]

Nothing springs to mind, sorry.

[flying-sheep]

OK, this should help. i’m logging the shit out of this and duplicating the code used to search for template paths

[flying-sheep]

uuh, my search path isn’t added:

['.',
 '.../site-packages/nbconvert/exporters/../templates',
 '.../site-packages/nbconvert/exporters/../templates/skeleton']

why? some overeager caching?

[flying-sheep]

wtf? why is only the python3 kernel found within those kernel dirs

def find_kernel_specs() contains only return KernelSpecManager().find_kernel_specs()

wait

before i can understand this, please explain how kernel_dirs gets set at all: i see no @default here…

[takluyver]

The @default decorator is a change in the latest version of traitlets, and that code hasn't been updated yet. It's relying on the older API where a traitlet named foo looks for a method _foo_default().

[takluyver]

I suspect the issue is that you need to add the test files to package_data here so that they get installed:

nbconvert/setup.py

Line 55 in a0a4a5f

'nbconvert' : [

[flying-sheep]

aaah, makes sense. but may i ask why you install tests at all?

[takluyver]

Different approaches - some people like to be able to install a package and run its tests, and Jupyter code is generally structured that way, with tests inside the package.

[flying-sheep]

OK. what do you say about this PR?

[takluyver]

Much the same as I said before:

• You're making two quite different changes, I'd be happier if they were two separate PRs we could discuss individually.
• I'd like to see some more relevant performance measurements before we rip out all the caching, because the tests are not designed for that. It might be fine, or we may want to keep the environment caching, and let Jinja handle template caching.
• I'd like to have a bit more discussion with the other devs about whether and how to implement kernel specific templates. E.g. is it better to allow kernels to override exporters, rather than templates? Could we handle the case you want this for better by adding more flexibility in the current template, plus something in notebook metadata? What are the merits of kernels overriding existing export formats versus adding new ones?

[flying-sheep]

i’m happy to split this, but as said: i didn’t get it to work with caching and i suspect this is at least partly due to bugs in caching.

so yeah, we should probably decide first if it’s worth to cache, before i adapt this all to caching only to have caching ripped out later for buggyness and uselessness..

Could we handle the case you want this for better by adding more flexibility in the current template, plus something in notebook metadata?

i thought about this as well and think it’s also a good idea. maybe we should extend the kernel.json spec to have a "resources": { ... } or "template_data": { ... } object that’s merged into the resources?

[jankatins]

Just to chime in: I'm also helping out in the development of a environmental kernel manager (register all conda/virtualenv environments as kernels, if they have irkernel or the ipykernel installed: https://github.com/Cadair/jupyter_environment_kernels). Part of that is that we currently (or better after Cadair/jupyter_environment_kernels#24) "set" our own kernel spec dir to get logos to the UI...

I'm not sure how hard it would be to find the right dir in the env itself, but this felt easier (and is also the thing which the current conda implementation of such an kernel env manager does).

[Carreau]

Tagging as closed-PR, and closing as this is seem to have stalled a bit.
This is just to keep the PR queue a bit shorter and focusing on what is actually worked on.
Feel free to reopen, (or ask for reopening if you don't have enough right to do so).
If you were not the original author, and want to revive this, please feel free to do so.

Also, we are doing our best to enable people to learn how to contribute to the project, and get commits in. If you are missing time or have any other issues that prevent you to work on that, please feel free to tell us, we can try to build on top of what you did.

Thanks a lot and looking forward to see this moving forward again.

[flying-sheep]

There was no progress as I was waiting for an answer to

so yeah, we should probably decide first if it’s worth to cache, before i adapt this all to caching only to have caching ripped out later for buggyness and uselessness.

So what about it? Is caching fixed? Is it going to be removed because it never worked properly in the first place and has no tangible performance impact?