PGVector#update_texts #248
Conversation
dcluna
force-pushed
the
update-texts-pgvector
branch
from
dce4c06 to
3df05fc
Compare
| upsert_texts(texts: texts, ids: ids) | ||
| end | ||
|
|
||
| def update_texts(texts:, ids:) |
There was a problem hiding this comment.
Would you please annotate this method as well?
|
@dcluna Please run |
dcluna
force-pushed
the
update-texts-pgvector
branch
from
73f4b01 to
7d409a3
Compare
There was a problem hiding this comment.
@andreibondarev Please take a look at this comment - had to write a corner case for when ids are not required in add_texts, but if this is not necessary I'd prefer to have the original API using the same method for both add/update operations.
| @@ -47,7 +47,44 @@ | |||
| it "adds texts" do | |||
| command = subject.add_texts(texts: ["Hello World", "Hello World"]) | |||
There was a problem hiding this comment.
The way this was written suggests that ids are optional in this method - however, from the usage I could find (for instance, https://github.com/andreibondarev/langchainrb/blob/7d409a378ab300f4641a71b74d25e5cd9593122f/lib/langchain/active_record/hooks.rb#L52-L55) it seems like this is not the case.
I'd be more than happy to enforce the existence of texts/ids in tandem, but not sure if there is an (upcoming?) use case that relies on this API.
|
@andreibondarev Also, not sure if CI did not trigger due to me force-pushing the branch or if it's actually manually triggered by you - in any case, could you please make it run so this PR can be merged? |
|
@dcluna No, at least at |
This commit implements (tentative) pgvector support for upsert functionality, which as far as I can understand is all that is required to make it work correctly with the Rails integration.
This is open to discussion and modification - please let me know if there's anything to improve here.