-
Notifications
You must be signed in to change notification settings - Fork 183
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
feat: add apiserver support to Python SDK #327
base: main
Are you sure you want to change the base?
Conversation
4ff9346
to
2539adb
Compare
try checkpoint add asgiref to support async-to-sync: fix unit tests remove pydantic warning add unit tests for client fix mock path test model added tests and fix discovered bugs fix connection error handling fix bugs surfaced in end-to-end use explicity properties fix awaitable checks add instance method revert to return sync class extract base model use instance() method on models add e2e tests working state fix unit tests more fixes
2539adb
to
e04269e
Compare
session = await deployment.client.get_session(session_id=session_def.session_id) | ||
for task_def in await session.get_tasks(): | ||
tasks.append(task_def) | ||
return JSONResponse(tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a dict of session_id -> list[task] ? (Just thinking about building a UI that would use this function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, many return payloads of the apiserver API are not well thought, we can start fixing it from here
|
||
Using the class constructor is not possible because we want to alter the class method to | ||
accommodate sync/async usage before creating an instance, and __init__ would be too late. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, this docstring answered so many questions. Thank you!
if asyncio.iscoroutinefunction(method) and not name.startswith("_"): | ||
setattr(Wrapper, name, async_to_sync(method)) | ||
# Static type checkers can't assess Wrapper is indeed a type[T], let's promise it is. | ||
return cast(type[T], Wrapper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just confirming how this works
- we define a class with all async methods
- if a flag is set, we apply this wrapper
- this wrapped only converts public methods
- type checkers and IDEs will see this as a sync method instead of async?
🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seems like IDE/type-checker assumes its always async (which tbh is fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah correct, didn't think about that so to clarify, the static checker would be happy with await client.sync.apiserver.status()
because the actual wrapping happens at runtime. I agree this would be fine, but I'll think about possible implications
llama_deploy/client/base.py
Outdated
class _BaseClient: | ||
"""Base type for clients, to be used in Pydantic models to avoid circular imports.""" | ||
|
||
def __init__(self, **kwargs: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged the settings into the client itself, making this work. If we like accessing settings like client.apiserver_url
, autocompletion works now.
pass | ||
|
||
|
||
class SessionCollection(Collection): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing a few methods? I don't see how I can create a new task under a session (just playing around a bit with the code in vscode, trying to understand the usage pattern)
Introducing apiserver support through a new Python SDK.
Basic usage
Streaming events