-
Notifications
You must be signed in to change notification settings - Fork 224
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 a basic async python client starting point #1014
Conversation
b76ffae
to
dbd316e
Compare
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.
PR is too big to not approve :)
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.
Have a few minor nits. Otherwise looks like a good foundation.
🤔 Since we are starting from scratch, it would be nice to enable mypy
from the beginning. I might make a PR for this later this week.
.github/workflows/pypi-publish.yml
Outdated
name: PyPI Publish | ||
name: Build and upload python wheels |
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.
For continuity of the jobs, I think it would be nice to keep the name the same. We already have enough dead GA names.
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've reverted this change so the workflow will still be the same. Do dead GA names matter for workflows? Or jobs? The job name has to change because its splitting one job into three.
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 think workflows. I'm mostly thinking of this left sidebar on https://github.com/lancedb/lancedb/actions
There some some dead names there (e.g. "Node Release"), which I find annoying.
region: Optional[str], | ||
host_override: Optional[str], | ||
read_consistency_interval: Optional[float], | ||
) -> Connection: ... |
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.
Doesn't this return a Future?
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 changed it to async def
which should imply the future. I'm on the fence about these having these pyi files at all and curious for your opinion. These methods aren't exposed to the user so this is only for our own internal understanding.
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.
For internal only things, I don't think we have to write them. Sometimes we might publicly expose them though, if the Python wrapper is redundant.
If I add actual mypy checks to the repo, my opinion on this might change.
0bdab9b
to
a2d818b
Compare
This changes `lancedb` from a "pure python" setuptools project to a maturin project and adds a rust lancedb dependency. The async python client is extremely minimal (only `connect` and `Connection.table_names` are supported). The purpose of this PR is to get the infrastructure in place for building out the rest of the async client. Although this is not technically a breaking change (no APIs are changing) it is still a considerable change in the way the wheels are built because they now include the native shared library.
This changes `lancedb` from a "pure python" setuptools project to a maturin project and adds a rust lancedb dependency. The async python client is extremely minimal (only `connect` and `Connection.table_names` are supported). The purpose of this PR is to get the infrastructure in place for building out the rest of the async client. Although this is not technically a breaking change (no APIs are changing) it is still a considerable change in the way the wheels are built because they now include the native shared library.
This changes `lancedb` from a "pure python" setuptools project to a maturin project and adds a rust lancedb dependency. The async python client is extremely minimal (only `connect` and `Connection.table_names` are supported). The purpose of this PR is to get the infrastructure in place for building out the rest of the async client. Although this is not technically a breaking change (no APIs are changing) it is still a considerable change in the way the wheels are built because they now include the native shared library.
This changes
lancedb
from a "pure python" setuptools project to a maturin project and adds a rust lancedb dependency.The async python client is extremely minimal (only
connect
andConnection.table_names
are supported). The purpose of this PR is to get the infrastructure in place for building out the rest of the async client.Although this is not technically a breaking change (no APIs are changing) it is still a considerable change in the way the wheels are built because they now include the native shared library.