ipython / ipykernel

IPython Kernel for Jupyter
https://ipykernel.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
653 stars 368 forks source link

Suggest to make implementations of some function always return awaitable #1295

Open Carreau opened 1 week ago

Carreau commented 1 week ago

See discussion in #1272; It is not deprecated, but being able to always know you can (and must) await should be simpler in the long run.

Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await.

In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken;

I changed some of the base methods to be async, but I'm happy to move those back to sync.

A few other things use the if awaitable(...): pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.

Carreau commented 1 week ago

I'm going to revert making the default implementation awaitable later, but currently this breaks some downstream things in the way I was expecting as consumers don't check wether the return is an awaitable or not.

minrk commented 1 week ago

I don't object to using 7.0 to make all base class do_ methods async. It is a breaking change, but a lot is already broken by 7.0. It is a difficult balance to strike, and I know I tend to come down on the side of not breaking things. The plus side is that subclasses can make these methods async without losing backward compatibility since async has been allowed for quite some time.

One thing I'll note is that allowing subclasses to implement methods as sync or async is one thing (what we do in ipykernel), whereas changing a base class implementation to be async or not (has happened some places in 7.0 I think, this PR makes it more consistent) is definitely a straightforwardly breaking change. It is not the case that the current API where subclasses may define methods as async means that the base class can switch between sync and async without it being considered a major breaking change. Allowing multiple implementations of these methods is really the point of the Kernel class, but so is a stable base implementation.

Carreau commented 1 week ago

I agree, right now I'm changing to 1) see the breakage (and it broke spyder kernel, that does not handle sync or async) 2) because it means that our own test emits warnings.

I don't like breaking things without warnings either; But I think that it's (too) easy to implement a subclass, or using a class without asking wether the methods could be async if the default implementation is sync; and I'd like to curb the complexity of navigating and understanding those codebase.

I don't touch them that often; but we had a couple of client request, and getting back into that for fixes or advices with all the conditional options makes it IMHO more challenging than it should be.