[ENH]: Force statistics wrapper callers to specify an output collection

[tanujnay112]

Description of changes

Summarize the changes made by this PR.

• Improvements & Bug fixes
  • attach_statistics_function(collection) -> attach_statistics_function(collection, "<output_collection_name>")
  • get_statistics(collection) -> get_statistics(collection, "<output_collection_name>")
• New functionality
  • ...

Test plan

How are these changes tested?

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

Migration plan

Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?

Observability plan

What is the plan to instrument and monitor this change?

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the _docs section?_

[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)

[tanujnay112]

• [ENH]: Change attach_function::function_id param to be clearer #5962
• [ENH]: Allow specifiying multiple filter keys in get_statistics #5963
• [ENH]: Force statistics wrapper callers to specify an output collection #5961 👈 (View in Graphite)
• [ENH]: Make detach a Collection method #5958 : 2 other dependent PRs (#5959 , #5960 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[propel-code-bot]

Explicit statistics output collection parameter introduced

Updates the statistics utilities to require callers to explicitly supply a statistics output collection name when attaching or retrieving statistics. Corresponding integration tests now provide explicit names and include minor typing cleanups, ensuring the new API contract is exercised.

Key Changes

• Changed attach_statistics_function signature to require stats_collection_name and removed default naming logic
• Updated get_statistics to require stats_collection_name input and adjusted documentation and examples accordingly
• Modified distributed statistics wrapper tests to pass explicit collection names throughout and added typing annotations for metadata preparation

Affected Areas

• chromadb/utils/statistics.py
• chromadb/test/distributed/test_statistics_wrapper.py

This summary was automatically generated by @propel-code-bot

[propel-code-bot]

[Logic] The previous implementation of get_statistics used get_statistics_fn(collection) to verify that the statistics function was actually attached to the collection before attempting to fetch the results. This check has been removed.

While the goal is to make the stats_collection_name explicit, removing this validation weakens the function's contract. It's now possible to call get_statistics on a collection where statistics are not enabled, or to pass a stats_collection_name that doesn't correspond to the one configured for the collection. This could lead to confusing CollectionNotFound errors or incorrect empty results if an unrelated collection with that name exists.

Consider reintroducing a check to ensure that statistics are enabled for the given collection, for example by calling get_statistics_fn(collection) before this block. This would make the function more robust and its behavior clearer.

Context for Agents

The previous implementation of `get_statistics` used `get_statistics_fn(collection)` to verify that the statistics function was actually attached to the collection before attempting to fetch the results. This check has been removed.

While the goal is to make the `stats_collection_name` explicit, removing this validation weakens the function's contract. It's now possible to call `get_statistics` on a collection where statistics are not enabled, or to pass a `stats_collection_name` that doesn't correspond to the one configured for the collection. This could lead to confusing `CollectionNotFound` errors or incorrect empty results if an unrelated collection with that name exists.

Consider reintroducing a check to ensure that statistics are enabled for the given collection, for example by calling `get_statistics_fn(collection)` before this block. This would make the function more robust and its behavior clearer.

File: chromadb/utils/statistics.py
Line: 186

[propel-code-bot]

[Reliability] The previous implementation of get_statistics verified that a statistics function was attached to the collection by calling get_statistics_fn. This check has been removed, which could lead to confusing behavior. For example, a user could accidentally pass the name of a regular collection to stats_collection_name and receive either an error about malformed statistics data or nonsensical results.

Consider reintroducing a validation step to ensure that a statistics function is attached and that it points to the provided stats_collection_name. This would make the function more robust and user-friendly.

For example, you could add a check at the beginning of the function:

from chromadb.errors import NotFoundError

try:
    af = get_statistics_fn(collection)
    if af.output_collection != stats_collection_name:
        raise ValueError(
            f"Statistics function for collection '{collection.name}' outputs to "
            f"'{af.output_collection}', not '{stats_collection_name}'."
        )
except NotFoundError:
    raise ValueError(
        f"Statistics are not enabled for collection '{collection.name}'. "
        f"Please call \`attach_statistics_function\` first."
    ) from None

This would restore the safety of the previous implementation while still respecting the new explicit stats_collection_name parameter.

Context for Agents

The previous implementation of `get_statistics` verified that a statistics function was attached to the collection by calling `get_statistics_fn`. This check has been removed, which could lead to confusing behavior. For example, a user could accidentally pass the name of a regular collection to `stats_collection_name` and receive either an error about malformed statistics data or nonsensical results.

Consider reintroducing a validation step to ensure that a statistics function is attached and that it points to the provided `stats_collection_name`. This would make the function more robust and user-friendly.

For example, you could add a check at the beginning of the function:

```python
from chromadb.errors import NotFoundError

try:
    af = get_statistics_fn(collection)
    if af.output_collection != stats_collection_name:
        raise ValueError(
            f"Statistics function for collection '{collection.name}' outputs to "
            f"'{af.output_collection}', not '{stats_collection_name}'."
        )
except NotFoundError:
    raise ValueError(
        f"Statistics are not enabled for collection '{collection.name}'. "
        f"Please call \`attach_statistics_function\` first."
    ) from None
```

This would restore the safety of the previous implementation while still respecting the new explicit `stats_collection_name` parameter.

File: chromadb/utils/statistics.py
Line: 125

[tanujnay112]

Merge activity

• Dec 9, 2:51 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Dec 9, 2:51 AM UTC: @tanujnay112 merged this pull request with Graphite.