Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better typing support #381

Closed
marcodlk opened this issue May 17, 2024 · 4 comments
Closed

Better typing support #381

marcodlk opened this issue May 17, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@marcodlk
Copy link
Contributor

Which project are you requesting an enhancement for?

kr8s

What do you need?

First of all, thank you for creating kr8s - I completely I agree with the stated motivation and was excited when I found it.

The only thing that's really missing currently in my view is the lack of typing support. From what I gather, the difficulty in trying to provide better typing support stems from the current design aiming to make a "mirrored" sync and async API available via kr8s.api and kr8s.asyncio.api. I am wondering if it would be possible to keep everything the same from the user's POV but refactor the internal design to allow for full typing support, at least for API objects and methods that users interact with.

The main issue right now imo is that the main sync API objects have no typing support. For instance, the following results in pod always being recognized as Any:

from kr8s.objects import Pod

pod = Pod("kube-apiserver", namespace="kube-system")

# Assisting type checker does not work, `pod` is still recognized as `Any`
pod: Pod = Pod("kube-apiserver", namespace="kube-system")

When trying to use kr8s, I ended up using kr8s._object instead of kr8s.object in imports when writing code to at least get the type checker to recognize methods and attributes. I would then switch the imports back to the real ones (kr8s._object) to run. This is not a good dev experience for users who have become accustomed to typing support.

Would a package like asyncer potentially be helpful here? Is there anything in the current sync + async compat design that needs to be taken into consideration?

I understand this would entail a significant refactor, but it is a bit of a blocker to devs like me, as much as I love everything else about the package. I would be happy to contribute to the refactor if there is an appetite, but it may take some time as I don't have a lot of time to allocate at the moment.

@marcodlk marcodlk added the enhancement New feature or request label May 17, 2024
@jacobtomlinson
Copy link
Member

Thanks for the kind words, and for taking the time to write this up. I would be very happy see typing support improved here.

Unfortunately I don't think I can personally justify the time to do a major refactor of the internals to satisfy a small group of users. But I'd be happy to chip away at it, and even happier to review and merge PRs that move things in this direction. If this issue gets a large number of 👍 reactions that may help motivate things though.

I had a quick look through asyncer and it doesn't seem to be doing anything particularly different to what we do here already in kr8s._io for wrapping async code. I'm apprehensive to overhaul that code or swap it out because it took a long time to get it working the way you would expect, especially in cases where users are using the kr8s sync API within an interactive async environment (like Jupyter or IPython). I don't think just using asyncer is the right thing to do, but if code written with asyncer behaves the way you would expect from a typing perspective then I'm sure there are things we can learn from it.

I think some helpful next steps would be to dig deeper into what behaviour you would expect, what's going wrong with it and most importantly why the type checker is behaving that way. Then we need a robust way of checking that things behave the way we expect from a user perspective, this may be simply running mypy or doing something else via tests.

Your Pod example is a great start, but honestly I'm a bit out of my depth and I have no idea what is going on. That class is defined in kr8s/objects.py and I find it susprising that a type checker recognises it as Any.

kr8s/kr8s/objects.py

Lines 185 to 188 in a7a5c3c

@sync
class Pod(_Pod):
__doc__ = _Pod.__doc__
_asyncio = False

The @sync decorator goes over all the methods of the base class and wraps the async methods up into sync ones. So perhaps the mere existance of the class decorator confuses the type checker?

Also I'm curious about your experience of importing from kr8s._objects. Those exact objects are exposed via the public API at kr8s.asyncio.objects. Why not use those? Do they not show the correct type information?

@jacobtomlinson
Copy link
Member

In #384 I fixed up some of the async wrapping code to make it generic, so the types get passed along correctly. This solves the specific problem being raised in this issue.

My next question is: what's next? What else can we do to improve typing support here?

@jacobtomlinson
Copy link
Member

The initial problem of not passing types through the async wrapping machinery has been resolved. All mypy errors have also been resolved in a series of Pull Requests.

Now that #421 is merged and we are ensuring good type hygiene I think we can close out this issue.

The next step may be to turn on strict mode in mypy, but that feels beyond the scope of this issue. If anyone has any other thoughts on improving typing here please feel free to open a new issue.

@marcodlk
Copy link
Contributor Author

marcodlk commented Jul 2, 2024

Apologies for the delayed response - this looks good, resolves the main problem which was sync API objects having no typing support. Thanks @jacobtomlinson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants