Raw template as settable attribute

[mpacer]

So, the two models of use that seem as simple as possible are:

from nbconvert.preprocessors import RSTExporter
class AttrExporter(RSTExporter):
    raw_template = """{%- extends 'rst.tpl' -%}
{%- block in_prompt -%}
blah
{%- endblock in_prompt -%}
"""

and

from nbconvert.preprocessors import RSTExporter
exp = RSTExporter()
exp.raw_template = """{%- extends 'rst.tpl' -%}
{%- block in_prompt -%}
blah
{%- endblock in_prompt -%}

But if you overwrite the traitlet, you lose access to all the nice traitlet features. So the following wouldn't work:

from nbconvert.preprocessors import RSTExporter
class AttrExporter(RSTExporter):
    raw_template = """{%- extends 'rst.tpl' -%}
{%- block in_prompt -%}
blah
{%- endblock in_prompt -%}

exp = AttrExporter()
exp.raw_template = ""
exp.template_file == RSTExporter().template_file #returns False
"""

But if you set raw_template to the empty string after defining it as a traitlet default, then it the template_file will revert to the classes' previous template_file traitlet default (or, technically, its default_template, if that is also set as a nonzero value). So this would work

from nbconvert.preprocessors import RSTExporter
from traitlets import  #default
class AttrExporter(RSTExporter):
    @default('raw_template')
    def _raw_template_default(self):
        raw_template = """{%- extends 'rst.tpl' -%}
{%- block in_prompt -%}
blah
{%- endblock in_prompt -%}

exp = AttrExporter()
exp.raw_template = ""
exp.template_file == RSTExporter().template_file #returns True
"""

edits for clarity & @takluyver's comments

[mpacer]

Ok that last bit should work now.

This can probably be simplified a bit but this has the heart of what I was thinking of in #673

Happy to change the name.

[takluyver]

I'm a bit uncomfortable that this is called both in _load_template() and in the observe handler. I haven't identified a failure case yet, but I suspect that this is going to make it harder to reason about what data is where at what point in execution.

In particular, I think that using a traitlet but encouraging people to override it in subclasses with a simple attribute is likely to be a source of confusion - I think the mechanism should either use traitlets entirely, or not involve a traitlet at all. (The latter is my preference, but that's because I generally think traitlets introduce more confusion than they remove)

[takluyver]

Don't you sleep? :-P

[mpacer]

Ok going to go to bed now… but using the FunctionLoader (rather than DictLoader) should make it easier to change things out under the hood which should make managing everything much more straightforward once I rework the logic to actually take that model account.

but thank you @takluyver for the review… while I didn't implement exactly the implementation you suggested, I have removed the expectation for magic to occur unless you are using traitlets.

Note that in the previous test introduced in the model of #490's use case, we already support this overwriting technique for template_file without breaking functionality. So my current approach is "let's not ban using it that way, but then you have no guarantees that anything fancier will work" and then not encourage people to use it that way (i.e., by not treating it as a traitlet).

[mpacer]

And @takluyver I should be asleep but I knew this would stick in my mind once I read your comments since they were worthwhile and I had a decent approach to address what I think is the core of your point. Which is that the state model I was using was a little all over the place(or that's at least my summary of the critique). This approach should make it much cleaner while still providing neat functionality that I think a lot of people would use via a super simple API.

[mpacer]

w00t! didn't realise that traitlets could run into race conditions if they are interdependent.

Was really hard to debug since testing interactively vs. non-interactively had different results. Fortunatley, I recognised that as a signal of race_conditions and hold_trait_notifications() is a glorious solution to that problem.

I feel pretty good about this now.

[takluyver]

Sorry, but I can't say I feel good about it. hold_trait_notifications() feels like a fix to a problem that we shouldn't be having in the first place. This is exactly the kind of reason I don't want to use traitlets; we shouldn't be struggling to debug race conditions just to handle templates provided as either a filename or a string.

This PR now introduces a new class, three new traitlets and two new methods on the existing class. #674, which aims to solve the same problem, adds one method and one instance attribute. The way you use them in a subclass is a bit different, but I don't think it's a clear advantage:

my_template = """..."""

# 675 recommended use: 6 lines involving traitlets
from nbconvert.preprocessors import RSTExporter
from traitlets import default
class AttrExporter(RSTExporter):
    @default('raw_template')
    def _raw_template_default(self):
        return my_template

# 675 non-recommended use: 3 lines, standard Python idioms
from nbconvert.preprocessors import RSTExporter
class AttrExporter(RSTExporter):
    raw_template = my_template

# 674: 5 lines, standard Python idioms
from nbconvert.preprocessors import RSTExporter
class AttrExporter(RSTExporter):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        self.use_string_template(my_template)

[takluyver]

And I know how satisfying it is to finally understand and fix a difficult bug. But step back and look at the bigger picture: the task we're coding for does not involve any concurrency - there are no threads or coroutines involved. The fact that you had to fix a race condition is a 'code smell', because that kind of bug shouldn't arise in this kind of code.

[takluyver]

Sorry if I'm sounding grumpy about this, by the way. I am a bit frustrated that you have spent all this effort implementing and debugging a more complex solution without suggesting any reasons why my simple solution in #674 does not do what we need.

[mpacer]

Hi, @takluyver, did you realise you can use #675's approach in a recommended way in 4 lines using standard python idioms?

from nbconvert.preprocessors import RSTExporter
class AttrExporter(RSTExporter):
    def __init__(self, **kwargs):
        super().__init__(raw_template=mytemplate, **kwargs)

Furthermore, this allows you to set raw_template when creating an instance in 1 line using standard python idioms (see test_raw_template_constructor:

exporter = RSTExporter(raw_template=mytemplate)

Additionally, you this allows the ability to inspect the content of your template without diving into private APIs (I figure this doesn't need a test to illustrate):

exporter = RSTExporter(raw_template=mytemplate)
print(exporter.raw_template)

Additionally, you're ignoring the feature my approach make possible, embodied in test_raw_template_init, the key lines being

exporter_deassign = RSTExporter()
exporter_deassign.raw_template = raw_template
output_deassign, _ = exporter_deassign.from_notebook_node(nb)
assert "blah" in output_deassign
exporter_deassign.raw_template = ''
assert exporter_deassign.template_file == 'rst.tpl'
output_deassign, _ = exporter_deassign.from_notebook_node(nb)
assert "blah" not in output_deassign

Specifically it allows you to set a raw_template by default and still recover the original template_file without issue.

And specifically, contrary to what you were saying, you do need to deal with race conditions to set templates if you want to enable this feature (if we continue to use traitlets for template_file). The order in which you define the raw_template, cache the previous template, and assign the _raw_template_key as the template matters as well as later when you deassign the raw_template, retrieve the previous template and assign it as the template. Because this is all occurring using traitlets that are now interdependent, this introduces race conditions. So, by your standard, using traitelts that are interdependent is a code smell, which I simply do not agree with (as they were written in order to be used in that manner).

Additional Notes:

One of the principles guiding my approach here was "Setting a template via providing a string should be as similar as possible to assigning a new template".

That means, if possible, the form of their APIs should be identical; specifically you should be able to set a template via a string by assignment to an attribute.

I have now used DictLoader but then need to invalidate the environment anytime raw_template is set, otherwise Jinja won't allow you to reassign templates. That was why I had originally added the ExplicitFunctionLoader†, and I realised that I didn't have a test for that behaviour. I have now added a test to demonstrate this behaviour (test_raw_template_reassignment).

So in terms of your structural analysis: I now introduce 1 new traitlet, 2 private attributes, and 2 1 private methods.

†: If we ever want to use the FunctionLoader, we need to create this in order to then allow using the list_templates function in our environment calls.

[mpacer]

@rgbkrk @Carreau @minrk would any of you want to review this?