[ENH] Cross-Version Collection Migration

[atroyn]

Description of changes

This PR creates a path to migrating from previous versions of Chroma to the new version where we have collection configuration storage. The migration is idempotent and non-destructive.

Since all collections now must have a configuration, old collections would error when loading them - this was reflected in cross-version persistence failures.

With this approach, that doesn't happen. This is a first step to providing user-facing migration tooling. For now it's just this one script, but later as we add more of these, they can be composed in a more intelligent way.

This PR includes a new CLI application as part of the chroma CLI, chroma migrate which will migrate all collections in a specified path (and optional tenant, and database), with ./chroma being the default.

Test plan

Manual Test:

• Create a collection using an old version of chroma with a persisted client
• Load the database with a persistent client from this version. list_collections() should fail with a JSON parsing error (since configurations don't exist)
• Run the migration function over the client from the new version.
• list_collections() should work as expected.

Automated:
test_cross_version_persist passes locally and in CI.

ALL TESTS Should pass by this point in the stack.

Documentation Changes

The migration and migration tool is documented at https://docs.trychroma.com/deployment/migration

Additionally, when a collection tries and fails to load a CollectionConfiguration from JSON, the error points the user to the same migration documentation.

TODO:

• Documentation of migration
• Migration tool in CLI

[github-actions]

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

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

[atroyn]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [ENH] Cross-Version Collection Migration #2400 👈
• [ENH] Distributed System collection config storage #2396
• [ENH] Collection Configuration Storage #2338
• [CLN] ServerAPI to use Collection model #2300 : 1 other dependent PR (#2265 )
• main

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

Join @atroyn and the rest of your teammates on Graphite

[HammadB]

I think its a bit odd that you have to specify the tenant and database, and would prefer if we added some tooling to migrate all of them. This feels like pushing our constraints (no list tenants) onto our users and delivering a worse UX.

[atroyn]

My thought here was that if you are using the tenant and database args, you know what you're doing and what you want to migrate. I could instead crawl the entire sysDB too. Let's discuss.

(the error here is I forget to pass these args, fixed.)

[HammadB]

do we want to version these? how does this extend in the future?

[atroyn]

Let's discuss. My thought here was we would wait for the next one to happen and then write the right abstraction, but you make the point elsewhere that we do already have other migrations. Will take a look at those.

[HammadB]

I think we should remove and deprecate https://github.com/chroma-core/chroma-migrate since we are adding this

[HammadB]

I think we may be able to run this migration as a sql migration? That would be preferable I think. I'm not really a fan of this UX and introducing a new migration hook that isn't future proof in a nice way - forcing people to run a migration tool for a change like this feels heavy

[atroyn]

doing this in SQL is scary because it would require direct string manipulation without validation.

[HammadB]

should we only run this if the old version is one we expect to fail?

[atroyn]

My thought is that any migration we run here must be idempotent in the case there's nothing to be done, and doing this test this way helps ensure that.