feat(chroma): update to chroma 1.x#3552
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughUpgrade Chroma integration to chromadb/chroma:1.4.0 and chroma-go v0.3.2 (API v2): update Docker image, heartbeat endpoint, client initialization and lifecycle, collection/add/query APIs, tests, examples, and module dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modules/chroma/examples_test.go`:
- Around line 221-225: The example test currently hardcodes the float distance
"0.7525849" in the Output block which is brittle; change the test to assert the
returned distance using a tolerance instead of exact string matching (e.g.,
compare the numeric distance from the query result with math.Abs(result.Distance
- expected) < tol) and update or remove the exact float in the Output comment;
look for the example that prints "Result of query" in
modules/chroma/examples_test.go and replace the strict equality check /
expected-output line with a tolerance-based numeric assertion referencing the
query result variable (e.g., result.Distance) and a small tolerance (e.g.,
1e-3).
🧹 Nitpick comments (5)
modules/chroma/chroma_test.go (1)
43-45: Unnecessarydeferinsidet.Cleanup, and should usett(subtest*testing.T).Two things:
- The
deferinsidet.Cleanupis redundant — the function body only callsClose(), sodeferadds no value. Just callchromaClient.Close()directly.- This uses
t(parent test) instead oftt(subtest). The cleanup will run when the outerTestChromafinishes rather than when the "GetClient" subtest finishes. Usett.Cleanupto scope the lifecycle correctly.Proposed fix
- t.Cleanup(func() { - defer chromaClient.Close() - }) + tt.Cleanup(func() { + chromaClient.Close() + })modules/chroma/examples_test.go (3)
214-219:defer Close()should be placed immediately after successful client creation.The
defer chromaClient.Close()is placed at the very end of the function (line 214), after all collection operations. If any operation between client creation (line 123) and here panics, the client leaks. Move it right after the error check on line 128.Proposed fix — move Close() defer right after client creation
Place this block right after line 128 (
returnafter failed client creation):defer func() { err = chromaClient.Close() if err != nil { log.Printf("failed to close client: %s", err) } }()And remove lines 214–219.
72-82: Same late-deferpattern here —Close()should follow client creation.The
defer Close()at lines 77–82 should be placed immediately after verifying client creation succeeded (after line 70), not after the heartbeat call. In example code this also serves as a best-practice demonstration for users.Proposed fix
chromaClient, err := chromago.NewHTTPClient(chromago.WithBaseURL(endpoint)) if err != nil { log.Printf("failed to get client: %s", err) return } + // closeClient { + // ensure all resources are freed, e.g. TCP connections or the default embedding function which runs locally + defer func() { + err = chromaClient.Close() + if err != nil { + log.Printf("failed to close client: %s", err) + } + }() + // } errHb := chromaClient.Heartbeat(context.Background()) - // } fmt.Println(errHb) // error is only returned if the heartbeat fails - // closeClient { - // ensure all resources are freed, e.g. TCP connections or the default embedding function which runs locally - defer func() { - err = chromaClient.Close() - if err != nil { - log.Printf("failed to close client: %s", err) - } - }() - // }
96-103: Minor: simplify mount slice construction.Proposed simplification
testcontainers.WithHostConfigModifier(func(hostConfig *container.HostConfig) { - dockerMounts := make([]mount.Mount, 0) - dockerMounts = append(dockerMounts, mount.Mount{ + hostConfig.Mounts = []mount.Mount{{ Type: mount.TypeBind, Source: filepath.Join(cwd, "v1-config.yaml"), Target: "/config.yaml", - }) - hostConfig.Mounts = dockerMounts + }} }),modules/chroma/go.mod (1)
6-7: Direct dependency ondocker/dockeradded for host config modifier usage in examples.The
examples_test.gofile directly importscontainerandmounttypes fromgithub.com/docker/dockerto configure bind mounts viaWithHostConfigModifier. While this works, testcontainers-go provides higher-level helpers (WithMounts,WithFiles) that could potentially simplify this and reduce the direct Docker SDK dependency in examples. Consider whether these alternatives could replace the currentWithHostConfigModifierapproach for mounting the configuration file.
Replace brittle exact float comparison with tolerance check to avoid CI failures from platform-dependent floating point precision differences.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modules/chroma/examples_test.go`:
- Around line 96-106: The example launches Chroma via chroma.Run and currently
passes an ALLOW_RESET env var while also mounting v1-config.yaml which already
sets allow_reset: true; remove the redundant WithEnv map entry for "ALLOW_RESET"
from the chroma.Run call (i.e., drop
testcontainers.WithEnv(map[string]string{"ALLOW_RESET": "true"})) or, if you
choose to keep it, add a short comment next to the chromaContainer/chroma.Run
invocation explaining that Chroma 1.x reads allow_reset from v1-config.yaml and
the environment variable is unnecessary and may be ignored to avoid misleading
users.
🧹 Nitpick comments (2)
modules/chroma/examples_test.go (2)
216-221: Movedefer chromaClient.Close()immediately after successful client creation.Currently,
Close()is deferred at line 216, but the client is created at line 124. Any early return between lines 129–215 (there are ~6 such paths) will skip registering the defer and leak client resources (e.g., TCP connections, local embedding function).♻️ Proposed fix
Move the defer block right after the client error check:
chromaClient, err := chromago.NewHTTPClient(chromago.WithBaseURL(endpoint)) // } if err != nil { log.Printf("failed to get client: %s", err) return } + defer func() { + err = chromaClient.Close() + if err != nil { + log.Printf("failed to close client: %s", err) + } + }() // reset {And remove the defer block at lines 216–221.
159-166: Remove commented-out code from example.Lines 160 and 184 contain commented-out options (
WithIDGenerator,WithInclude). Since this is user-facing example code meant to be copied, leaving dead comments can be confusing. Either remove them or convert to explanatory comments if the intent is to show alternative usage.
The mounted v1-config.yaml already sets allow_reset: true, making the env var unnecessary for Chroma 1.x.
tazarov
changed the title
Replace WithHostConfigModifier bind mount with testcontainers.WithFiles for better reproducibility. Move v1-config.yaml to testdata directory.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/chroma/examples_test.go`:
- Around line 204-209: The defer that calls chromaClient.Close() is registered
too late and won't run if any early return happens; immediately after creating
and validating chromaClient (right after the error check that follows the client
construction/assignment), add a deferred closure that calls chromaClient.Close()
and logs any error (i.e., defer func() { if err := chromaClient.Close(); err !=
nil { log.Printf("failed to close client: %s", err) } }()), and remove the later
deferred block that currently appears around lines 204–209 so Close() runs even
on early returns.
- Around line 89-94: The ContainerFile passed into chroma.Run (the
testcontainers.ContainerFile literal in the chromaContainer creation) is missing
FileMode and thus creates a no-permission file; update that ContainerFile to
include FileMode: 0o644 so the container can read /config.yaml (modify the
ContainerFile used in the chroma.Run call).
🧹 Nitpick comments (2)
modules/chroma/examples_test.go (2)
73-81: Consider placingdefer Close()immediately after successful client creation.The
defer chromaClient.Close()is registered at the very end of the function. If future edits add an early return between client creation (line 64) and here, the client won't be closed. Moving the defer right after the error check on line 68 is the idiomatic Go pattern.That said, I see this is grouped in a
// closeClient {documentation section — if preserving that marker placement is important for doc generation, this is acceptable as-is.
147-154: Remove commented-out code from example functions.Lines 148 and 172 contain commented-out API calls. In example code that users copy, these are confusing — either include them with a clear explanation or remove them entirely. If they're meant as documentation hints, consider adding them to the module's doc page instead.
…r early Set FileMode 0o644 on the copied config file so the container can read it. Move chromaClient.Close() defer immediately after client creation to ensure cleanup on early returns.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modules/chroma/examples_test.go`:
- Around line 153-160: Remove the stale commented-out snippet and any references
to undefined symbols: delete the commented
chroma.WithIDGenerator(chroma.NewULIDGenerator()) line inside the col.Add(...)
call and ensure you do not reference non-imported/undefined types like
types.IDocuments; if you intended to use chroma-go symbols, replace any
accidental "chroma." references with the correct "chromago." identifiers (e.g.,
use chromago's ID generator API) and/or remove the erroneous import/use so the
example compiles cleanly.
🧹 Nitpick comments (1)
modules/chroma/examples_test.go (1)
187-188: Consider splitting this long line for readability in example code.At ~170 characters, this line packs a lot of logic (index access, type conversion, tolerance check, and format string) into a single
fmt.Printf. For documentation examples that users will reference, splitting it would improve clarity.♻️ Suggested split
- distance := queryResults.GetDistancesGroups()[0][0] - fmt.Printf("Result of query: %v %v distance_ok=%v\n", queryResults.GetIDGroups()[0][0], queryResults.GetDocumentsGroups()[0][0], math.Abs(float64(distance)-0.7525849) < 1e-3) + ids := queryResults.GetIDGroups()[0] + docs := queryResults.GetDocumentsGroups()[0] + distances := queryResults.GetDistancesGroups()[0] + distanceOk := math.Abs(float64(distances[0])-0.7525849) < 1e-3 + fmt.Printf("Result of query: %v %v distance_ok=%v\n", ids[0], docs[0], distanceOk)
…m-v2 * upstream/main: (269 commits) chore(deps): bump actions/checkout from 6.0.1 to 6.0.2 (#3560) chore(deps): bump go.opentelemetry.io/otel/sdk to v1.41.0 (#3589) feat: add TiDB module (#3575) feat: add Forgejo module (#3556) feat: improve container conflict detection (#3574) chore(deps): bump go to 1.25 everywhere (#3572) chore(pulsar): bump base image to 4.x, replacing the wait for log strategy with wait for listening port (deterministic) (#3573) chore(deps): bump github.com/sigstore/sigstore in /modules/compose (#3571) chore(compose): update to compose-v5 (#3568) chore(deps): bump github.com/modelcontextprotocol/go-sdk (#3557) chore(deps): bump mkdocs-codeinclude-plugin from 0.2.1 to 0.3.1 (#3561) chore: update usage metrics (2026-03-02) (#3565) chore(deps): bump mkdocs-include-markdown-plugin from 7.2.0 to 7.2.1 (#3562) chore(deps): bump go.opentelemetry.io/otel/sdk in /modules/grafana-lgtm (#3563) chore(deps): bump go.opentelemetry.io/otel/sdk in /modules/toxiproxy (#3564) feat(azure): add lowkey vault container (#3542) feat(chroma): update to chroma 1.x (#3552) chore(deps): bump mkdocs-include-markdown-plugin from 7.2.0 to 7.2.1 (#3547) chore(deps): bump tj-actions/changed-files from 47.0.0 to 47.0.1 (#3546) chore(deps): bump actions/upload-artifact from 4.6.2 to 6.0.0 (#3545) ...
2 participants
What does this PR do?
Updates the Chroma module from chromadb/chroma:0.4.24 to chromadb/chroma:1.4.0 and bumps chroma-go from v0.1.2 to v0.3.2.
Chroma 1.x ships a v2 REST API, so this PR:
Why is it important?
The module was pinned to Chroma 0.4.x, which is no longer maintained. Chroma 1.x changed the REST API and Go client in backwards-incompatible ways, so the module stopped working against current Chroma releases.
Related issues
N/A