-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[ENH]: clarify non-included fields in .get() and .query() responses #2028
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
064fd90
to
017b1ea
Compare
017b1ea
to
315a680
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. This is a change to our API and some of our users may be surprised -- could you put a note in https://docs.trychroma.com/migration about it? I don't think we need to do email comms but we should definitely do discord comms.
|
w00t! |
@codetheweb should we standardize this and add it to JS as well? (separate PR works for me) |
to be honest, I'm not sure, I feel like (good) Python SDKs tend to use helper classes whereas JS SDKs tend to use plain objects without magic behavior; but maybe it makes sense for consistency we could also improve the typings of the JS SDK so the return type is dependent upon the |
Closing in favor of #2044. |
Description of changes
Closes #300.
print(collection.get(...))
now looks like this:Could use some opinions on this--is it too verbose? I figured it would be better to be consistent across all fields affected by
include
, but we could scope this to justembeddings
for now as well.This can probably be rolled into 0.5 as a potentially breaking change (
embeddings is None
will no longer work).Test plan
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Existing docs don't really give much details on how the response shape changes depending on
include
, so not necessary to update but might be nice to give additional details.