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

Replace @sync decorator with APIObjectSyncMixin for all sync objects #551

Merged
merged 12 commits into from
Jan 15, 2025

Conversation

jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Jan 14, 2025

Supersedes #493
Closes #491

This change explores stubbing out the sync API with correct typing instead of using the @sync decorator.

Under the hood the @sync decorator wraps public coroutines with sync methods, however static analysis tools aren't clever enough to know this and so tools like mypy complain about them being coroutines even though they aren't.

In this PR all sync objects have been updated to explicitly wrap each coroutine in a sync method which allows us to correctly set the type annotations. Thanks to the suggestion from @ion-elgreco I had a go at using a mixin to stub out the base class methods. Having to stub out methods like .create() for every object is what put me off this approach, but using the mixin means this only needs doing once and it seems to work fine.

# test.py
from kr8s.objects import Pod
Pod.list()
$ mypy test.py   
Success: no issues found in 1 source file

TODO:

  • Stub out objects
  • Stub out port forward
  • Stub out Api
  • Update auto class generation logic

Sorry, something went wrong.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 85.18519% with 32 lines in your changes missing coverage. Please review.

Project coverage is 94.05%. Comparing base (87063fc) to head (2003e9b).
Report is 160 commits behind head on main.

Files with missing lines Patch % Lines
kr8s/objects.py 77.90% 19 Missing ⚠️
kr8s/_objects.py 90.24% 8 Missing ⚠️
kr8s/__init__.py 85.71% 3 Missing ⚠️
kr8s/_portforward.py 83.33% 1 Missing ⚠️
kr8s/portforward.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
- Coverage   94.61%   94.05%   -0.57%     
==========================================
  Files          29       30       +1     
  Lines        3141     4156    +1015     
==========================================
+ Hits         2972     3909     +937     
- Misses        169      247      +78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ion-elgreco
Copy link

Let me know if you need a reviewer!

@jacobtomlinson
Copy link
Member Author

@ion-elgreco sure that would be great! Any feedback would be appreciated.

)

@classmethod
async def async_get(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, but _async might be a better name in terms of UX. If someone wants to do a certain action they start typing .get and both the sync and async version then will show up.

Copy link
Member Author

@jacobtomlinson jacobtomlinson Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trouble with making the async methods private is that sometimes one object wants to call an explicitly async method on another object.

For example the Service.async_ready_pods() method wants to call await Pod.ready(), but given that this may be replaced with a sync method it calls await Pod.async_ready() instead.

return [pod for pod in pods if await pod.async_ready()]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I follow you here. The async methods don't have to be private.

I think every function should be design be async, but then over the whole api surface you have sync versions of them.

So it's fine to keep async_ready_pods, and have that call pod.async_ready. But the function ready_pods should only run async_ready_pods in a synchronous fashion

Copy link
Member Author

@jacobtomlinson jacobtomlinson Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, I misunderstood what you meant by _async.

In this case I disagree that users should be directly calling the async_ methods. They are semi-private methods and are only intended for use by kr8s internals. Ideally I would like to call them _async_get but then one object can't call that method on another object (without the linters being angry).

We aren't trying to expose both a sync and async API in the same object, this would be a pretty unpleasant API for users. Instead we provide two versions of the same object and depending on where you import it from it's either sync or async.

# sync
from kr8s.objects import Pod

po = Pod.get("foo")
# async
from kr8s.asyncio.objects import Pod

po = await Pod.get("foo")

It's unlikely that a single codebase will want to use both APIs. It's probably also unlikely that a single user will want to use both APIs, some people will want sync, others will want async.

I don't want users of the async API to have a poor experience and have to do things like this:

# bad pattern
from kr8s.objects import Pod

po = await Pod.async_get("foo")

Copy link

@ion-elgreco ion-elgreco Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtomlinson I think actually having separate objects with the same exact name that have the async methods will be extremely confusing. Having a single Class that can do async and sync is imho more clear than having to import the same class from a different subclass.

Now if I need to be able to call both sync and async in the same script, I would have to import the classes differently:

from kr8s.objects import Pod as SyncPod or from kr8s.asyncio.objects import Pod as AsyncPod

Copy link
Member Author

@jacobtomlinson jacobtomlinson Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think actually having separate objects with the same exact name that have the async methods will be extremely confusing. Having a single Class that can do async and sync is imho more clear than having to import the same class from a different subclass.

This is the way kr8s has always been designed. I'm not open to changing it at this stage.

As the zen of Python says "Namespaces are one honking great idea -- let's do more of those!".

Now if I need to be able to call both sync and async in the same script, I would have to import the classes differently

I think it would be highly unusual to need to do that. My experience of users of this library is that people only want one or the other. But yeah if you wanted to import both you would need to rename them at import, the tests do this a lot.

I think the sync API gives you what you're after though, you have both sync and async methods. I hear what you're saying about switching async_get to get_async, but I'm not sure it's worth the pain at this point. If the kr8s.asyncio namespace adds confusion then you can just ignore its existence.

However, for folks who ONLY want an async experience then the API provided at kr8s.asyncio is much cleaner for them. Forcing them to prefix async_ on every method they call is not very pleasant.

kr8s/objects.py Outdated Show resolved Hide resolved
@jacobtomlinson
Copy link
Member Author

I realise that I keep mentioning the @sync decorator in the commit messages here, but that may be causing GitHub to ping @sync every time I do. Sorry if that's the case Anthony!

@jacobtomlinson jacobtomlinson marked this pull request as ready for review January 15, 2025 17:28
@jacobtomlinson jacobtomlinson merged commit 0d74bab into kr8s-org:main Jan 15, 2025
13 checks passed
@jacobtomlinson jacobtomlinson deleted the sync-mixin branch January 15, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MyPy errors when using sync objects
2 participants