Conversation
|
@o-santi could you pls review this one |
|
Yes, of course. Just give me a sec to read all that 😅 |
o-santi
left a comment
o-santi
left a comment
There was a problem hiding this comment.
Looks good to me, just a small nitpick on introducing a deprecated method from the get go.
|
|
||
| return await collection._create_if_not_exists() | ||
|
|
||
| @deprecated("use AsyncClient.get_or_create_collection") |
There was a problem hiding this comment.
Why create a method that is already deprecated? Isn't it better to just not it include it at all, if it's going to be removed in the near future. No one depends on it, and including would just make removing it later harder.
There was a problem hiding this comment.
Oh yes, I should have mentioned this, sorry (I rushed it a bit). The private method _does_collection_exists uses get_collection. What should be the best way to "fix this"? I can update _does_collection_exists to use the text query for checking if collection exists?
There was a problem hiding this comment.
sync _does_collection_exists can be:
query = text(
"""
select 1
from
pg_class pc
join pg_attribute pa on pc.oid = pa.attrelid
where
pc.relnamespace = 'vecs'::regnamespace
and pc.relkind = 'r'
and pa.attname = 'vec'
and not pc.relname ^@ '_'
and pc.relname = :name
"""
).bindparams(name=name)
with client.Session() as sess:
result = sess.execute(query).fetchone()
return result is not None
async:
query = text(
"""
select 1
from
pg_class pc
join pg_attribute pa on pc.oid = pa.attrelid
where
pc.relnamespace = 'vecs'::regnamespace
and pc.relkind = 'r'
and pa.attname = 'vec'
and not pc.relname ^@ '_'
and pc.relname = :name
"""
).bindparams(name=name)
async with client.AsyncSession() as sess:
result = await sess.execute(query)
row = result.fetchone()
return row is not None
There was a problem hiding this comment.
The thing is also that inside tests after deletion get_collection is used to assert that collection indeed doesn't exists . We'll not be able to test anymore with this, I guess I can use sql query in tests too, just wanna make sure is this the approach we want to take?
There was a problem hiding this comment.
Hey, there haven't been many updates on this pull request, when do you expect it to be merged and released on pypi :c? @diksipav
There was a problem hiding this comment.
hey @F4k3r22 I believe the PR can only be merged by the Supabase team @o-santi
There was a problem hiding this comment.
Oh :(, Let's hope he merges it as soon as possible @o-santi
There was a problem hiding this comment.
I'm so sorry, I had to sort through some personal issues the last week and did not give enough attention to this PR. For this specific issue, I think creating a function _exists (no need for the does_collection prefix) is the best course of action, and its a good idea to while you're at it make the sync version use it too.
There was a problem hiding this comment.
Hey, thanks so much for checking it out. I hope those changes will be made and merged soon :D
|
Hey everyone, I need to know if the changes from this pull are already integrated into the main branch and if it's already on Pypi. I have a RAG project and would like to support Vecs to connect to Supabase and perform the necessary operations asynchronously |
| async def __len__(self) -> int: | ||
| """ | ||
| Returns the number of vectors in the collection. | ||
|
|
||
| Returns: | ||
| int: The number of vectors in the collection. | ||
| """ | ||
| async with self.client.AsyncSession() as sess: | ||
| async with sess.begin(): | ||
| stmt = select(func.count()).select_from(self.table) | ||
| result = await sess.execute(stmt) | ||
| return result.scalar() or 0 | ||
|
|
There was a problem hiding this comment.
We should not override dunder methods, for their meaning is special cased in the python interpreter. Having a __len__ method might make the user call len() on this object, only to face a TypeError for it returning a coroutine instead of an integer. This should be a _len method.
| async def __getitem__(self, items: str): | ||
| """ | ||
| Asynchronously fetches a vector from the collection by its identifier. | ||
|
|
||
| Args: | ||
| items (str): The identifier of the vector. | ||
|
|
||
| Returns: | ||
| Record: The fetched vector. | ||
|
|
||
| Raises: | ||
| ArgError: If the input is not a string. | ||
| KeyError: If no vector is found with the given ID. | ||
| """ | ||
| if not isinstance(items, str): | ||
| raise ArgError("items must be a string id") | ||
|
|
||
| row = await self.fetch([items]) | ||
|
|
||
| if not row: | ||
| raise KeyError("no item found with requested id") | ||
|
|
||
| return row[0] |
There was a problem hiding this comment.
Same thing applies to this dunder method.
5 participants
What kind of change does this PR introduce?
Support for the async Client. Feature: #6 .
What is the current behavior?
You can't use the Client with async operations.
What is the new behavior?
Both sync and async operations are supported.
I added tests and also tested basic operations locally, but there may be things I missed. Also not sure if the tests are written in the best way.