Using existing exceptions defined and newly defined exceptions to alter behavior of the get_collection, create_collection and delete_collection` methods

[bhashithe-air]

Description of changes

get_collection, delete_collection and create_collection methods raised a ValueError. This was descriptive for the user to find what the error was but was inconvenient because it was the same Exception type for two different scenarios.

Resolves #982

Summarize the changes made by this PR.

• Improvements & Bug fixes
  • Scenario 1 is when the user tries to get or delete a collection which does not exist. There was an exception already created in chromadb.errors called InvalidCollectionException we raise that in the two methods.
  • Scenario 2 is when a user tries to create a collection which already excepts. To this we created a new error in chromadb.errors called CollectionAlreadyExistsError and raise that in the respective function
• New functionality
  • Raising proper exceptions

[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 (Readbility, Modularity, Intuitiveness)

[tazarov]

@bhashithe-air, this is great but can we make things a bit more RESTful-ish by adding the response code. Here is an example:

class CollectionNotFoundError(ChromaError):
    @overrides
    def code(self) -> int:
        return 409  # Conflict with current state

    @classmethod
    @overrides
    def name(cls) -> str:
        return "CollectionNotFound"

[bhashithe-air]

Yes let me add this part in as well

[tazarov]

@bhashithe-air thank you very much for the PR. I think the issue you have identified around the consistency of the API is important with people deploying Chroma extensively in a Client/Server mode.

Please have a look at my review comments regarding response codes and a new error for collection not found.

[bhashithe-air]

@tazarov Thanks!

What do you think about the InvalidCollectionException, currently for many situations where the error "collection not found"; InvalidCollectionException is being used. Should I change it to reflect the error you suggest CollectionNotFoundError?

[tazarov]

@tazarov Thanks!

What do you think about the InvalidCollectionException, currently for many situations where the error "collection not found"; InvalidCollectionException is being used. Should I change it to reflect the error you suggest CollectionNotFoundError?

Well, it's about consistency, cognitive load and intuitive of the code. While I understand your point InvalidCollectionException does seem counter-intuitive and does make one wonder what is exactly not valid about the collection. Let's wait for @HammadB's feedback and then we plan further changes (or no changes).

[bhashithe-air]

It does seem counter intuitive yes, i am in favor of your suggestion to use a new exception. But yes, we shall wait for @HammadB :)

[HammadB]

I agree the InvalidCollectionException is underdefined, I prefer NotFound and more prescriptive errors.

[tazarov]

@bhashithe-air, It looks better now :).

One suggestion for future PRs - please setup GPG and sign your commits

[bhashithe-air]

Thanks @tazarov, just learnt about the GPG signing for commits, I will make sure to sign my commits in the future.

Do you think anything else is needed for this PR? should i look at the whole repo and see if we need to define anything new?

[tazarov]

LGTM. There are many other things to "fix" on the API but this is a step in the right direction. @HammadB wdyt?

[tazarov]

@bhashithe-air, can you rebase off main so tests can pass?

[bhashithe-air]

@tazarov I synced just now.

[jeffchuber]

@tazarov what is the latest on this?

[tazarov]

@bhashithe-air, can you rebase/merge from the main so things are up to date, or give me write access to your repo so that I can do it?

[bhashithe-air]

@tazarov I added you to the repository. Do not have too much time to work on it today.

[tazarov]

@tazarov I added you to the repository. Do not have too much time to work on it today.

Thank you @bhashithe-air, I will rebase this shortly.

[tazarov]

@bhashithe-air, I redid your PR from a non-main branch in a new PR #1618. Feel free to close this one.