Fix: Inherit from LoggingConfigurable

[martinRenou]

cc. @maartenbreddels

This was removed for performance concern, but it seems that people were relying on it being an LoggingConfigurable class.

Fix ipython/ipykernel#1026

[maartenbreddels]

Can’t we do this in ipykernel? So comm has no dependency on traitlets?

[martinRenou]

It has the dependency on the Traitlets package already, even though it does not provide trait classes for now.

I don't have a strong opinion whether or not to do this in ipykernel, @blink1073 what do you think?

[blink1073]

I think it makes sense to use HasTraits here so downstream users don't have to remember to add it as a base class. I think we should add a test where the comm is used as an Instance() trait to make sure we don't hit the parent attribute problem.

[martinRenou]

Indeed I will add a test

[maartenbreddels]

I strongly believe we shouldn't have a traitlets dependency in this package if we want to keep the comm package light weight. We can do multiple inheritance in ipykernel, which is the only place we need to be a LoggingConfigurable for backwards compatibility, or simply accept the parent argument and discard it.
A cleaner method would be to do composition, if we don't like multiple inheritance.

[martinRenou]

@maartenbreddels we can probably discuss this in a separate issue later and fix ipykernel with this for now

[maartenbreddels]

See ipython/ipykernel#1028
What do you think?

[maartenbreddels]

Note that this PR still doesn't make it fully backward compatible, because it does not inherit from LoggingConfigurable.

Also, the whole point of the comm package IMHO, is to get rid of traitlets, and make it really lightweight. If we again inherit from any traitlets class, the comm package would again have to break compatibility in the future. I think it's ipykernel's responsibility to deliver backward compatibility.

[martinRenou]

Note that this PR still doesn't make it fully backward compatible, because it does not inherit from LoggingConfigurable.

Right! I went a bit too fast, I fixed it

[martinRenou]

Also, the whole point of the comm package IMHO, is to get rid of traitlets

Nothing stops you from providing your own Comm implementation that does not depend on traitlets. That's what xeus-python will do.

[maartenbreddels]

Yes, but now we will still have bad performance in ipywidgets, because LoggingConfigurable is quite slow. If we use ipython/ipykernel#1028 we can have the ipykernel Comm class be backwards compatible, but the create_comm function create our light weight comm object (which is what we can do in ipywidgets).

[martinRenou]

but the create_comm function create our light weight comm object (which is what we can do in ipywidgets).

Are you saying you'd like ipywidgets to provide its own comm implementation? If yes, I strongly disagree with this, the kernel should provide the comm implementation, not libraries that make use of the comms. If we were to do that, kernels like xeus-python and pyolite (jupyterlite) would need to patch ipywidgets so that it does not interfere with the proper implementations.

[maartenbreddels]

No, ipywidgets can use the create_comm which does not have to be inherited from LoggingConfigurable. I think only ipykernel.comm.Comm should be backwards compatible, that's why we need ipython/ipykernel#1028.

[maartenbreddels]

This last thing wasn't in yet btw, but I added it in ipython/ipykernel@5032761

It's all about having the new API be clean, and still have ipykernel.comm.Comm be backward compatible.

Does that make sense?

[martinRenou]

I see!

So basically with your PR:

• ipykernel still provides a backward compatible ipykernel.comm.Comm and ipykernel.comm.CommManager that people can use
• people using the new comm.create_comm benefit from the new traitlets-free API

Is that correct?

[maartenbreddels]

Yes

[maartenbreddels]

I wouldn't say this is great, it makes everything complex, but this is the price to pay to stay backward compatible. And maybe we should put a deprecation warning in the backward compatible Comm class, and tell people to use create_comm.

[martinRenou]

Yes a deprecation warning sounds good.

I like it! Thank you for opening a PR in ipykernel. I'm turning my PR into draft.

[martinRenou]

Closing as replaced by ipython/ipykernel#1028