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.

Sign up for GitHub

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

Jump to bottom

Adding support for Pgvector #74

Merged
merged 2 commits into from
Dec 5, 2023
Merged

Adding support for Pgvector #74

merged 2 commits into from
Dec 5, 2023

Conversation

sebastienblanc
Copy link
Contributor

Based on this discussion : #46

this is still in draft but will make the review and discussions easier

@sebastienblanc sebastienblanc requested a review from a team as a code owner November 28, 2023 15:03
@geoand
Copy link
Collaborator

geoand commented Nov 28, 2023

cc @jmartisk

@cescoffier cescoffier self-requested a review November 28, 2023 15:44
@geoand
Copy link
Collaborator

geoand commented Nov 28, 2023

Thanks a lot @sebastienblanc!

What do you still want to do on this?

@geoand
Copy link
Collaborator

geoand commented Nov 28, 2023

I assume this means that when the user configures:

quarkus.datasource.username = user
quarkus.datasource.password = pass
quarkus.datasource.jdbc.url = jdbc:postgresql://somehost:5432/some_db

the datasource is used to configure PgVector as well?

@sebastienblanc
Copy link
Contributor Author

I assume this means that when the user configures:

quarkus.datasource.username = user
quarkus.datasource.password = pass
quarkus.datasource.jdbc.url = jdbc:postgresql://somehost:5432/some_db

the datasource is used to configure PgVector as well?

Yes ! It takes the default datasource, named datasources is not something I added yet.

@geoand
Copy link
Collaborator

geoand commented Nov 28, 2023

Great!

named datasources is not something I added yet.

We don't need to worry about that for now

cescoffier
cescoffier requested changes Nov 28, 2023
View reviewed changes
pgvector/README.md Outdated Show resolved Hide resolved
pgvector/deployment/pom.xml Outdated Show resolved Hide resolved
...c/main/java/io/quarkiverse/langchain4j/pgvector/deployment/Langchain4jPgvectorProcessor.java Outdated Show resolved Hide resolved
...t/src/test/java/io/quarkiverse/langchain4j/pgvector/test/Langchain4jPgvectorDevModeTest.java Outdated Show resolved Hide resolved
...ployment/src/test/java/io/quarkiverse/langchain4j/pgvector/test/Langchain4jPgvectorTest.java Outdated Show resolved Hide resolved
pgvector/runtime/src/main/java/io/quarkiverse/langchain4j/pgvector/PgVectorEmbeddingStore.java Outdated Show resolved Hide resolved
pgvector/runtime/src/main/java/io/quarkiverse/langchain4j/pgvector/PgVectorEmbeddingStore.java Outdated Show resolved Hide resolved
.../src/main/java/io/quarkiverse/langchain4j/pgvector/runtime/PgVectorEmbeddingStoreConfig.java Outdated Show resolved Hide resolved
pgvector/runtime/src/main/resources/META-INF/quarkus-extension.yaml Outdated Show resolved Hide resolved
pgvector/runtime/src/main/resources/META-INF/quarkus-extension.yaml Outdated Show resolved Hide resolved
@cescoffier
Copy link
Collaborator

For the tests you can reuse the once from Redis.

jmartisk
jmartisk reviewed Nov 29, 2023
View reviewed changes
pgvector/runtime/pom.xml Outdated Show resolved Hide resolved
jmartisk
jmartisk approved these changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@jmartisk jmartisk left a comment

Choose a reason for hiding this comment

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

Nice, technically all looks good to me now.
We're missing documentation and the option to choose a non-default datasource

@sebastienblanc
Copy link
Contributor Author

I will add soon some doc. Do we want also a simple sample project ? like we have here https://github.com/quarkiverse/quarkus-langchain4j/blob/main/samples/chatbot/src/main/java/io/quarkiverse/langchain4j/sample/chatbot/IngestorExample.java ?

@geoand
Copy link
Collaborator

geoand commented Nov 30, 2023

. Do we want also a simple sample project ? like we have here https://github.com/quarkiverse/quarkus-langchain4j/blob/main/samples/chatbot/src/main/java/io/quarkiverse/langchain4j/sample/chatbot/IngestorExample.java ?

Yeah, that would be nice.

Also note, that we'll the commits to be squashed and rebased when done.

@sebastienblanc
Copy link
Contributor Author

. Do we want also a simple sample project ? like we have here https://github.com/quarkiverse/quarkus-langchain4j/blob/main/samples/chatbot/src/main/java/io/quarkiverse/langchain4j/sample/chatbot/IngestorExample.java ?

Yeah, that would be nice.

Also note, that we'll the commits to be squashed and rebased when done.

@geoand Will try to add this early next week, I had a busy end of the week.
For the doc is a page like this generated or do I need to write it manually https://github.com/quarkiverse/quarkus-langchain4j/blob/main/docs/modules/ROOT/pages/includes/quarkus-langchain4j-redis.adoc ?
*

@geoand
Copy link
Collaborator

geoand commented Dec 1, 2023

👍

That doc is generated

@geoand
Copy link
Collaborator

geoand commented Dec 4, 2023

I suspect this needs a rebase :)

@sebastienblanc
Copy link
Contributor Author

Yeah I did one this weekend but another is needed (and realized my git rebase skills were a bit rusty :) )

@sebastienblanc
Copy link
Contributor Author

@geoand This looks better, I need to squash all the commits now , right ?

@geoand
Copy link
Collaborator

geoand commented Dec 4, 2023

Yeah, that would certainly be nice :)

@sebastienblanc
Copy link
Contributor Author

@geoand I squashed that beast

@geoand
Copy link
Collaborator

geoand commented Dec 4, 2023

@jmartisk can you make sure this works in native mode?

@jmartisk
Copy link
Collaborator

jmartisk commented Dec 4, 2023 •
edited
Loading

We don't yet have any integration tests related to embedding stores, but yeah I can look into creating some infrastructure for that
UPDATE: reported #87

@geoand
Copy link
Collaborator

geoand commented Dec 4, 2023

For now just a sample manual test would be fine

@jmartisk
Copy link
Collaborator

jmartisk commented Dec 4, 2023

Ok gimme 15 minutes I'll try it

@geoand
Copy link
Collaborator

geoand commented Dec 4, 2023

Thanks a lot!

@geoand geoand requested a review from cescoffier December 4, 2023 13:55
@jmartisk
Copy link
Collaborator

jmartisk commented Dec 4, 2023

Ok, I've added a reflective class registration, it seems to work now (tried both inserting and retrieving)

sebastienblanc and others added 2 commits December 5, 2023 09:23
@sebastienblanc @geoand
Adding pgvector as embedding store
5fa2db0 
adding pgvector as embedding store

detect if pgvector is installable

make sure the exception is related to the missing extension

make dimension config property mandatory

Update pgvector/runtime/src/main/resources/META-INF/quarkus-extension.yaml

Update pgvector/runtime/src/main/resources/META-INF/quarkus-extension.yaml

various refactoring based on feedback and added a real test

deleted readme

Update pgvector/deployment/src/main/java/io/quarkiverse/langchain4j/pgvector/deployment/Langchain4jPgvectorProcessor.java

remove useless description

Pinecone embedding store

adding pgvector as embedding store

add documentation

generated config doc and added pgvector to doc's pom

pgvector store
@jmartisk @geoand
Make pgvector work in native mode
4e2a205
@geoand
Copy link
Collaborator

geoand commented Dec 5, 2023

I rebased this onto the latest main

@jmartisk jmartisk merged commit 21a3fb1 into quarkiverse:main Dec 5, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@jmartisk jmartisk jmartisk approved these changes

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sebastienblanc @geoand @cescoffier @jmartisk