-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
JS/TS + Misc Feedback #254
Comments
@transitive-bullshit this is really great, thanks for writing this up! Here is a quick set of reactions
🫡 great!
we have been discussing either moving to, or also supporting row-based insert in addition to column-based insert. This because the ergonomics are much more natural for a non-data-science and app-dev focused use case. This +1s that!
really great, thank you, we will update
right now chroma does not handle auth. What do you think is best to do here? Have "db-like" auth similar to how you would do something with
that's really interesting feedback... i hadnt heard that before! going to need to think about that. :)
none yet! but after the refactor, #214, lands - something we could def do! |
@transitive-bullshit do you think its better to support named and positional args (non-breaking change) or hard break change over to named? Not sure what the std convention is here in the JS world. |
Ideally, the only version you'd support would be named args. It's simpler, and at imho at this stage of your roadmap, this level of breaking change will be welcomed by JS/TS devs as long as it's communicated properly. Positional args for anything more than one or maybe 2 positions in JS/TS are pretty frowned upon imho. Feel free to double check w/ @sw-yx |
I agree with @transitive-bullshit. Named args will also make it easier to avoid future breaking changes or awkward function calls. |
@transitive-bullshit @rileytomasek @sw-yx named args it is! thanks everyone :) |
ONE ARG TO RULE THEM AND IN DARKNESS BIND THEM |
In this PR - docstrings! - types! - named params! This is a breaking change to the JS Client - it switches from positional to named arguments originally brought up by @transitive-bullshit in #254 Example: ``` // before const collection = await chroma.createCollection("test"); //after const collection = await chroma.createCollection({ name: "test" }); ``` This is especially nice when you are only using a few params. This PR does not bump the `npm` version number - we will cut a new release on Monday. We will need to land an update to our docs at that time as well. TBD
Implemented basically all of this! Thanks again @transitive-bullshit for the awesome issue 🎉 |
Hey @jeffchuber, I'm just going to throw a lot of random feedback at you mainly from a JS/TS dev's perspective focused on DX.
Feel free to ignore my suggestions – I just want to try and give you an honest brain dump that will hopefully be helpful. The more important stuff is bolded.
JS/TS SDK
axios
as a dependency;fetch
is a web standard and is the default for node.js >= 18 and all serverless/edge environments. here is a good resource that I've linked several other repos to for how to handlefetch
as a library author that is future proof for where the node.js community is headedundefined
as a positional argument, there's something wrong w/ the SDK 😉metadata
is already plural, sometadatas
is a lil awkwarde.g.,
add
function upserts documents instead of strictly creating new documentsadd
to take in a single typed array, with an optional second argument for optional paramsgetCollection
to support an optional typescript generic so all collection methods are properly typedSome example TS pseudocode that shows an idealized version of the package in action:
Here's a solid Pinecone TS wrapper by @rileytomasek which does a lot of similar things: https://github.com/rileytomasek/pinecone-client
Docs
let collection2 = await client.getCollection("my_collection", embedding_function=emb_fn)
)import
vs commonjsrequire
which are more or less deprecated going forwards (e.g., the embeddings page)snake_case
which is very pythonic and 99% of the JS/TS community usescamelCase
(api_key
for instance)ChromaClient
constructor, but I'm guessing this feature doesn't exist yet? maybe add a note to the docs for now?collection.query({ embeddings: [...] })
separate fromcollection.queryDocuments({ documents: [...] })
or something to make the difference explicit, since right now it's confusing that I could pass both and unclear which one would actually be usedwhere
vswhere_document
– I like that it's explicit here about the differenceHope this feedback is helpful && again, feel free to ignore & prioritize as you wish 🙏
The text was updated successfully, but these errors were encountered: