Is it possible to use HttpClient with Chroma in Vanna?

[andreped]

Is your feature request related to a problem? Please describe.
As seen in the documentations of Chroma, PersistentClient, which is the only storage solution Vanna's Chroma instance supports, is not recommended for production use (see here). In-memory store called EphemeralClient has the same problem.

The documentations recommend using an HttpClient instead (see here). That way the Chroma instance is remote and communication is performed through an endpoint. This also allows for multiple clients accessing the same server.

Describe the solution you'd like
Vanna should allow all three Chroma clients through setting the client type in the config which then is used to construct the Chroma instance inside Vanna.

If there are any other ways to change the Chroma Client, which still allow me to use Vanna, I am very interested to know how :]

---

EDIT: I am open to making a PR to address this.

[zainhoda]

Thanks @andreped ! If you'd like to make a PR for this, that would be amazing!

[andreped]

Thanks @andreped ! If you'd like to make a PR for this, that would be amazing!

I can make a PR to allow in-memory quite easily. Adding support for the HttpClient is a bit more tricky and likely requires some API discussion. So I suggest making two separate PRs for this, where we do the HttpClient stuff in another PR. Sounds good?

[zainhoda]

Two PRs is totally fine with me! Thanks!

…

On Wed, Feb 21, 2024 at 10:35 AM André Pedersen ***@***.***> wrote: Thanks @andreped <https://github.com/andreped> ! If you'd like to make a PR for this, that would be amazing! I can make a PR to allow in-memory quite easily. Adding support for the HttpClient is a bit more tricky and likely requires some API discussion. So I suggest making two separate PRs for this, where we do the HttpClient stuff in another PR. Sounds good? — Reply to this email directly, view it on GitHub <#249 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWQVKR4ZAJKZS657W57NQ3YUYH4RAVCNFSM6AAAAABDSSLHPKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJWHE3TEOJXGQ> . You are receiving this because you commented.Message ID: ***@***.***>

[andreped]

Do you have any suggestion on how we can best add support for HttpClient? See here for how it works in Chroma.

From the link above, the two fundamental difference are that for PersistentClient we do:

def PersistentClient(path: str = "./chroma",
                     settings: Settings = Settings()) -> API

While for HttpClient we do:

def HttpClient(
    host: str = "localhost",
    port: str = "8000",
    ssl: bool = False,
    headers: Dict[str, str] = {},
    settings: Settings = Settings()) -> API

[zainhoda]

@andreped I think if we just take in an optional chroma_client as a config parameter then we should be able to just use that directly whether it's EphemeralClient, PersistentClient or HttpClient. If no chroma_client is passed, then fall back to the current method of creating a PersistentClient?

[andreped]

@andreped I think if we just take in an optional chroma_client as a config parameter then we should be able to just use that directly whether it's EphemeralClient, PersistentClient or HttpClient. If no chroma_client is passed, then fall back to the current method of creating a PersistentClient?

Oh, thats actually what I did in a separate project already :P So thats sounds good! Will do that in the other PR.

---

EDIT: @zainhoda Is there a reason why CIs are no longer ran for PRs? I see for my new PR that tests are no longer performed:

vanna/.github/workflows/tests.yml

Line 4 in b3d46dc

push:

Do you want to add this? It is quite common to run tests for new PRs or when changes have been made to PRs. If you want. I can make a PR to add this, but you likely removed this for a reason? Oh, I guess you dont want to exhaust your own resource?
https://github.com/vanna-ai/vanna/blob/main/.github/workflows/tests.yml#L26

[andreped]

As this was fairly easy, I bundled both suggestions into the same PR: #250.

I am unable to run these tests locally, as my windows laptop does not allow me to run pre-commit and tests appropriately. I will run these on my macbook when I get home.

[andreped]

Hmm, how am I to run tests if I do not have these following three keys?
https://github.com/vanna-ai/vanna/blob/main/tests/test_vanna.py#L18

Would it be possible to do this solely using the Vanna e-mail I created? Hence, I am at least able to get the Vanna key. But more tricky with OpenAI and Mistral. Any thoughts?

Perhaps it would be better to implement unit tests in the near future instead of full-blown integration tests that require these keys?

[zainhoda]

I’ve been researching this and unfortunately it doesn’t seem like there’s an easy way to run the tests using repository secrets when you’re not a member of the repository. It’s a GitHub security mechanism. I’ve thought about putting some mocks in so that we have a subset of tests that are run against a mock implementation that doesn’t need any API keys. I’m not totally sure if that would be particularly useful though since most of what Vanna does is serve as a connector for various components. For now, don’t worry about the tests. I’ve set the tests to run on merge into main and I’m going to follow a procedure of accepting PRs into main but then make sure that the tests pass before creating a release.

…

On Wed, Feb 21, 2024 at 12:05 PM André Pedersen ***@***.***> wrote: Hmm, how am I to run tests if I do not have these following three keys? https://github.com/vanna-ai/vanna/blob/main/tests/test_vanna.py#L18 Would it be possible to do this solely using the Vanna e-mail I created? Hence, I am at least able to get the Vanna key. But more tricky with OpenAI and Mistral. Any thoughts? — Reply to this email directly, view it on GitHub <#249 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWQVKRLXNSWIRHAHFPFQMDYUYSPLAVCNFSM6AAAAABDSSLHPKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJXGMZDOOBRHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[andreped]

For now, don’t worry about the tests.

OK, great! But then the PR is ready to review. If you see something immediately off, let me know :] I can do the linting when I get home on my other laptop, if necessary.

[andreped]

Merged in #250.

Will be part of upcoming release.