ExecutePreprocessor: Get kernel name when executing, and not during initialization

[mgeier]

Since there doesn't seem to be any progress in PR #886, I'm proposing this more minimalist approach to fixing #878 (that I've already suggested in #886 (comment)).

The underlying design problems (as mentioned in #886 (comment)) are of course still there, but this PR at least fixes the regression introduced in #816. Hopefully this can be incorporated in the next nbconvert release!

If desired, I can make another PR at a later point to fix those other problems, but I think that is not as urgent as the fix in this PR.

[MSeal]

I think I'm ok with this change (though a test would be nice). We can always finish 886 if we want a more exhaustive implementation. @mpacer , thoughts?

[mpacer]

This has some issues from a traitlets perspective since it will no longer be a traitlet if set in this way. That shouldn't be an issue per se since it's primarily acting as a default value and there isn't any @observe logic here. Note if anyone is using that aspect of traitlets downstream, this will break it.

I would prefer a test of all expected behaviour if this is to be merged.

[mgeier]

Good point @mpacer, I didn't think about this!

What's the proper way to set the value of a traitlet on a class instance?

I've come up with two ways:

        if not self.kernel_name:
            ExecutePreprocessor.kernel_name.__set__(self, self.nb.metadata.get(
                'kernelspec', {}).get('name', 'python'))

        if not self.kernel_name:
            self._trait_values['kernel_name'] = self.nb.metadata.get(
                'kernelspec', {}).get('name', 'python')

Both work for me, but which is the proper one?
Or is there a different way to do it?

[SylvainCorlay]

This has some issues from a traitlets perspective since it will no longer be a traitlet if set in this way.

I think that it would still be a trait. This change merely removes the default initialization and sets a value later.

self._trait_values['kernel_name']

I would recommend not to use private functions of traitlets!

[mgeier]

Oh, looks like @SylvainCorlay is right:

import traitlets

class Foo(traitlets.HasTraits):
    bar = traitlets.Int()

x = Foo(bar=55)
x.bar = 99
print(x._trait_values)

This prints:

{'bar': 99}

So it indeed seems to be still a trait(let), otherwise the _trait_values dict would not reflect the latest change, right?

So this means my PR can stay unchanged?

About the tests y'all were mentioning: can someone else please write them?

[MSeal]

I can copy M's test over and get it merged independently if it helps.

[MSeal]

Got a soft +1 from M in person. Will merge and will pop up a unittest here in a minute.

[SylvainCorlay]

👍

[mgeier]

Thanks a lot @MSeal and all others that were involved, it's great to have a working master branch again!

Are there any plans yet for a new nbconvert release?

[MSeal]

I was going to see what should finish getting merged this week and get a smaller release going.

[mgeier]

@MSeal That sounds great, thanks!