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.

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

The test example doesn't appear to work any more (in the README.md) #150

Open
joeplatform48 opened this issue Aug 1, 2024 · 4 comments
Open
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@joeplatform48
Copy link

The example code doesn't appear to work.

The pkg/embeddeding/openai is not in the latest release.

The format for the the new collection is incorrect.

Currently is:

newCollection, err := client.NewCollection(
		context.TODO(),
        "test-collection",
		collection.WithMetadata("key1", "value1"),
		collection.WithEmbeddingFunction(openaiEf),
		collection.WithHNSWDistanceFunction(types.L2),
	)

But needs to be:

	newCollection, err := client.CreateCollection(context.Background(), 
                 "test-collection",
		collection.WithMetadata("key1", "value1"),
		collection.WithEmbeddingFunction(openaiEf),
		collection.WithHNSWDistanceFunction(types.L2),

And running gives the following errors:

Embedding dimension 378 does not match collection dimensionality 1536

I guess I need need to set something somewhere to make this generate correctly?

Thanks

@tazarov
Copy link
Contributor

tazarov commented Aug 2, 2024

@joeplatform48, thanks for this. I've been doing some major refactoring and improvements. The pkg/embeddedings/ is the new location for all embedding functions from v 0.2.0. Let me check on the examples to make sure they all work with the current release as 0.2.0 will take another week or so.

wrt to the error you get Embedding dimension 378 does not match collection dimensionality 1536, this usually happens when you have a collection created with OpenAI embeddings and then you try to update or query it with another embedding function.

@tazarov tazarov self-assigned this Aug 2, 2024
@tazarov tazarov added the documentation Improvements or additions to documentation label Aug 2, 2024
@tazarov
Copy link
Contributor

tazarov commented Aug 2, 2024

@joeplatform48, I see where the confusion comes from.

In earlier versions, I used a pretty static method to create collections CreateCollection, which took on a bunch of parameters, and some of those can be nils, etc. This did not feel right, nor very go-like, so I added the NewCollection func that accepts a bunch of options, which made things a little more builder-like and easier to understand code. The issue with this refactor was that I wanted to keep backward compat so I left the old CreateCollection func. That, in light of your issue, makes me think that I should get rid of the CreateCollection to eliminate the confusion that having two ways of creating a collection brings.

Just to make it clear NewCollection is available in the latest release v0.1.4.

Let me know if the above makes sense to you.

Your error regarding the dimensions is unrelated to the code confusion I've created with keeping CreateCollection around.

@joeplatform48
Copy link
Author

joeplatform48 commented Aug 2, 2024

Thanks for the response, it looks like the openai has moved up a level to here (or has not moved into embeddings yet):

"github.com/amikos-tech/chroma-go/openai"

and to solve the dimension issue in the example I used:

newCollection, err := client.CreateCollection(context.Background(), collectionName, metadata, true, *openaiEf*, types.L2)

but that was on me using CreatCollection, not the NewCollection.

I think with the example provided:

newCollection, err := client.NewCollection(
		context.TODO(),
        "test-collection",
		collection.WithMetadata("key1", "value1"),
		collection.WithEmbeddingFunction(openaiEf),
		collection.WithHNSWDistanceFunction(types.L2),
	)

you need an option for the collection name so this works:

newCollection, err := client.NewCollection(
		context.TODO(),
                 collection.WithName("test-collection"),
        	collection.WithMetadata("key1", "value1"),
		collection.WithEmbeddingFunction(openaiEf),
		collection.WithHNSWDistanceFunction(types.L2),
	)

But all good, it works a treat :)

@tazarov
Copy link
Contributor

tazarov commented Aug 2, 2024

@joeplatform48, I had a version of this with collection.WithName("test-collection"). However, one thing that struck me with this approach is that WithX is really an option to which a default can be set. This is not the case with collection name, it is required 100% of the time. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants