[BUG] Modify list_collections client methods to return a list of collection…

[itaismith]

Description of changes

list_collections currently returns collection objects. Using these objects can lead to errors, since some collections may have been created with embedding functions that are not available at the time of calling to list_collections. This PR modifies list_collections to return only collection names, until we have EF persistence.

Test plan

How are these changes tested?

• Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Docstrings and reference updated.

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[tazarov]

@itaismith, this feels extremely (and possibly unnecessarily) breaking change. I'll do some research on this but I feel that there are tons of apps with the following snippet of code:

for collection in client.list_collections():
  # do something with collection.name or collection.id or collection.metadata (e.g. filter based on name or some metadata)
  # or any of the other methods count(), add(), delete() ...

It seems to me that people have gotten around the issue that this PR is trying to address e.g. use a collection from list_collection to add data an expecting the embedding function to work out of the box.
However I think that the burden of this temporary workaround will cause more damage and time wasted even if in practice it is several lines of code (list + get_collection()). Furthermore isn't introducing a breaking change just so that we can revert back to original behavior in due course thus making it double work for anyone depending on list_collections()?

[atroyn]

@tazarov we should make this breaking change because the default behavior is incorrect. When we fix the behavior, we can then have users do the right thing.

We should prefer to break sooner than later, and support users to the extent we can in the meantime. @itaismith we might want to be clever here and make a wrapper for str where if you try to call any collection methods on the name we return we give you an informative error and point you at the migration guide.

[HammadB]

As a courtesy maybe we bump to a non patch version to ship this

…

On Mon, Dec 2, 2024 at 10:19 AM Anton Troynikov ***@***.***> wrote: @tazarov <https://github.com/tazarov> we should make this breaking change because the default behavior is incorrect. When we fix the behavior, we can then have users do the right thing. We should prefer to break sooner than later, and support users to the extent we can in the meantime. @itaismith <https://github.com/itaismith> we might want to be clever here and make a wrapper for "str" where if you try to call any collection methods on the name we return we give you an informative error and point you at the migration guide. — Reply to this email directly, view it on GitHub <#3216 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKW32MPUSNV2VBYLWNGVQD2DSQCJAVCNFSM6AAAAABS2BF5Y6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJSGM2DANJTG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

[atroyn]

I want to be sure we aren't shadowing str functionality inadvertently, otherwise looks good to me minus nits.

[atroyn]

Should read Client.get_collection("{name}")

Collection.{name} is confusing and not something anywhere in our API.

[itaismith]

name here is probably a confusing argument name. It is the attribute a user would try to access on the collection name string. So:

collection = client.list_collections()[0]
collection.add(...)

The error would be:

Use get_collection to access Collection.add

[atroyn]

Yes, I see now. I think this is confusing and should be made clearer Maybe:

"Use Client.get_collection({collection_name}) to call {method_name}"

[atroyn]

This implementation is a bit funky because str doesn't have a __getattr__ but does have __getattribute__ which is subtly different. I'm concerned that we would be shadowing str method calls accidentally;

• can we verify that this doesn't do anything unexpected with e.g. .join
• if it doesn't, can we comment on how this works at the level of method dispatch in each case?

[itaismith]

It is funky indeed. I can switch to explicit function definitions.

In any case this is a detailed description of the dispatch order:

When a user will try to access an attribute on a CollectionName string, the __getattribute__ method of str is invoked first. If a valid str method or property is found, it will be used. Otherwise, the fallback __getattr__ defined here is invoked next. It will error if the requested attribute is a Collection method or property.

For example:

collection_name = client.list_collections()[0] # collection_name = "test"

collection_name.startsWith("t") # Evaluates to True.

__getattribute__ is invoked first, selecting startsWith from str.

collection_name.add(ids=[...], documents=[...]) # Raises the error defined in this class.

__getattribute__ is invoked first, not finding a match in str. __getattr__ from this class is invoked and raises an error

[atroyn]

It's funky because if this was just str, __getattr__ would never get called (and doesn't exist) - this is handled by __getattribute__ checking for it. I think that's fine but we should make some sort of comment.

[atroyn]

maybe explicitly name this variable now to indicate it's a name not an actual collection

[atroyn]

LGTM pending nit.

[atroyn]

we know collection_name here and should put it in the error string.