JS clients have custom exceptions equivalent to python

[jeffchuber]

https://github.com/chroma-core/chroma/blob/main/chromadb/errors.py

We can mirror these errors in JS and then catch them and use them in JS - that way the JS client will have much nicer errors rather than barfing up the string the backend passes through.

[bbytiger]

@jeffchuber is this done? mind if I take?

[jeffchuber]

@bbytiger this is not done yet , but this would be really great!

users frequently get confused with the verbosity of errors that come back from FastAPI. I think for common errors from FastAPI and from JS (especially cant connect) - we want to make named errors that also provide useful instructions on what to do.

the JS client does do a fair amount of sanitization - so the most common error by far is that the backend is not running or is corrupted for some reason.

[bbytiger]

@jeffchuber tested the following locally for the "can't connect" case by turning off my docker daemon

const chroma = new ChromaClient({ path: "http://localhost:8000" });
await chroma.reset();

However, it hangs for 300 seconds before hitting the default timeout. Going to add a lower default timeout (say 15s), then throw timeout error, good with you?

[kevinlu1248]

Hey @bbytiger and @jeffchuber , I prompted Sweep AI to give a shot at this at https://github.com/chroma-core/chroma/pull/815, wondering what you think!

You can also reply to Sweep at https://github.com/kevinlu1248/chroma/pull/5 and Sweep will make corresponding changes!

[jeffchuber]

@bbytiger yea - let's do that!

[jeffchuber]

@kevinlu1248 close - but not the spirit of the issue. i think i would need to write more detailed issues for this to work

[kevinlu1248]

@jeffchuber Can you clarify what's missing so I can re-prompt Sweep?

I double-checked and seems like the proposed JS error types match up with the Python types.

Python error types: https://github.com/chroma-core/chroma/blob/b5065f4dd579b51691c9abd4b808f1b98e897839/chromadb/errors.py#L6-L57 Proposed JS error types: https://github.com/kevinlu1248/chroma/blob/2f56c50e10d82289c5b9ea9624f2252b0f2617bf/clients/js/src/errors.js#L1-L41

[jeffchuber]

@bbytiger did you still want to take a stab at this? lmk if not and I can release it back to the community

[bbytiger]

@jeffchuber apologies didn't have time to get to this one, feel free to release back to community thanks

[jeffchuber]

@bbytiger sure thing! thanks for letting me know

[ibratoev]

@jeffchuber I am looking for a good first issue to contribute. Is this still active - if so, please assign it to me. I will dig a bit in the code today for more context.

[ibratoev]

OK, I reviewed the code and I think I have a clear idea what needs to be done.

Problem:

• The JS client lacks error conversion similar to the Python client (see raise_chroma_error). In the Python client, when there is an error, it is mapped to a ChromaError* based on body.error and body.message.

JS Client Situation:

• JS client is a bit of a mess due to the generated API client. There is a chromaFetch function that maps error but it is not actually used and the mapping does not work the same way.

Implementation options:

1. The easiest implementation with the least amount of changes: Implement the mapping option in the chromaFetch. Configure the generated API to use it accordingly.
2. Explicit implementation following the Python client: Implement a raiseChromaError function and explicitly convert generated API errors in each thin wrapper method.
3. Get rid of the api generation and implement it cleanly following the Python code. This is the most work but would remove complexity of long generated code that is not really needed. Chroma API is small and it seems code generation brings more complexity than value.

I lean towards 1 to minimize changes as this is my first PR ;) @jeffchuber @tazarov Opinions? OK with implementing Option 1?

[ibratoev]

And it seems the JS client unit tests are failing and they are not executed as part of the CI. After a quick research: it seems this is a regression in: #1970

[ibratoev]

And it seems this PR (#1970) broke the chromaFetch usage. A lint step would have caught this issue. I can add that as well.

[ibratoev]

@jeffchuber I opened a first PR to start improving the error consistency: https://github.com/chroma-core/chroma/pull/2048