feat: replace hand-rolled embedding providers with pgedge-go-llm-lib (PR 2 of 4)#173
Open
dpage wants to merge 5 commits into
Open
feat: replace hand-rolled embedding providers with pgedge-go-llm-lib (PR 2 of 4)#173dpage wants to merge 5 commits into
dpage wants to merge 5 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
💤 Files with no reviewable changes (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR consolidates embedding provider clients (Voyage, OpenAI, Ollama) to use the shared ChangesEmbedding Provider Library Consolidation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant libProvider
participant llmClient as llm.Client
participant Upstream
Caller->>libProvider: NewProvider(cfg)
libProvider->>llmClient: llm.NewClient(options)
Caller->>libProvider: Embed(ctx, text)
libProvider->>llmClient: Embed(ctx, text)
llmClient->>Upstream: provider-specific embed request
Upstream-->>llmClient: embedding response []float64
llmClient-->>libProvider: embedding []float64
libProvider->>libProvider: set cached dimension if 0
libProvider-->>Caller: embedding []float64
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 48 complexity · 2 duplication
Metric Results Complexity 48 Duplication 2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
dpage
force-pushed
the
feat/llm-lib-pr2-embeddings
branch
from
a264223 to
b117629
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/chat/llm_tracing.go (1)
27-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale package reference in tracing comment.
The comment still says tracing is controlled by the embedding-package log level, but this code now uses the chat package log level. Please update this wording to avoid confusion during maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/chat/llm_tracing.go` around lines 27 - 30, Update the stale comment for tracingRoundTripper to say that request/response trace logging is controlled by the chat package log level (not the embedding-package log level); locate the comment above the tracingRoundTripper type in llm_tracing.go and replace the phrase "embedding-package log level" with "chat package log level" and adjust wording to state that trace logging occurs when the chat package log level is Debug or Trace so the comment accurately reflects current behavior.
🧹 Nitpick comments (2)
internal/embedding/libprovider_test.go (1)
144-147: ⚡ Quick winAssert
Embederror in lazy-dimensions test.The test currently discards the
Embederror, which can hide the real failure source. Failing fast on error will make this test much easier to diagnose.Suggested diff
- _, _ = p.Embed(context.Background(), "x") + if _, err := p.Embed(context.Background(), "x"); err != nil { + t.Fatalf("Embed: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/embedding/libprovider_test.go` around lines 144 - 147, The test is discarding the error from p.Embed which can mask failures; update the test in libprovider_test.go to capture the returned error from p.Embed (e.g., err := p.Embed(...)) and fail the test immediately if err != nil (use t.Fatalf or t.Fatal with the error), before asserting p.Dimensions() == 8, referencing the Embed method and Dimensions() on p.internal/embedding/libprovider.go (1)
112-114: ⚡ Quick winUse CAS to enforce one-time dimension initialization.
Load()==0followed byStore()is racy for “first successful Embed wins” semantics under concurrent calls.CompareAndSwap(0, d)makes the intent explicit and lock-free.Suggested diff
- if d := int32(len(vec)); d > 0 && p.dim.Load() == 0 { - p.dim.Store(d) - } + if d := int32(len(vec)); d > 0 { + p.dim.CompareAndSwap(0, d) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/embedding/libprovider.go` around lines 112 - 114, The current one-time dimension init uses p.dim.Load() == 0 followed by p.dim.Store(d) which is racy; replace that logic with an atomic CompareAndSwap so only the first successful embed sets the dimension. Locate the block where d := int32(len(vec)) and p.dim.Load()/Store() are used (in libprovider.go) and change it to attempt p.dim.CompareAndSwap(0, d) so the first goroutine wins and others won't overwrite the stored dimension.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/chat/llm_tracing.go`:
- Around line 27-30: Update the stale comment for tracingRoundTripper to say
that request/response trace logging is controlled by the chat package log level
(not the embedding-package log level); locate the comment above the
tracingRoundTripper type in llm_tracing.go and replace the phrase
"embedding-package log level" with "chat package log level" and adjust wording
to state that trace logging occurs when the chat package log level is Debug or
Trace so the comment accurately reflects current behavior.
---
Nitpick comments:
In `@internal/embedding/libprovider_test.go`:
- Around line 144-147: The test is discarding the error from p.Embed which can
mask failures; update the test in libprovider_test.go to capture the returned
error from p.Embed (e.g., err := p.Embed(...)) and fail the test immediately if
err != nil (use t.Fatalf or t.Fatal with the error), before asserting
p.Dimensions() == 8, referencing the Embed method and Dimensions() on p.
In `@internal/embedding/libprovider.go`:
- Around line 112-114: The current one-time dimension init uses p.dim.Load() ==
0 followed by p.dim.Store(d) which is racy; replace that logic with an atomic
CompareAndSwap so only the first successful embed sets the dimension. Locate the
block where d := int32(len(vec)) and p.dim.Load()/Store() are used (in
libprovider.go) and change it to attempt p.dim.CompareAndSwap(0, d) so the first
goroutine wins and others won't overwrite the stored dimension.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 644484c4-3ccb-476b-9b1b-53f550093979
📒 Files selected for processing (15)
docs/changelog.mdinternal/chat/llm_loglevel.gointernal/chat/llm_tracing.gointernal/chat/llm_tracing_test.gointernal/embedding/libprovider.gointernal/embedding/libprovider_test.gointernal/embedding/logger.gointernal/embedding/logger_test.gointernal/embedding/ollama.gointernal/embedding/ollama_test.gointernal/embedding/openai.gointernal/embedding/openai_test.gointernal/embedding/provider.gointernal/embedding/voyage.gointernal/embedding/voyage_test.go
💤 Files with no reviewable changes (9)
- internal/embedding/openai.go
- internal/embedding/voyage_test.go
- internal/embedding/ollama.go
- internal/embedding/logger_test.go
- internal/embedding/ollama_test.go
- internal/embedding/logger.go
- internal/embedding/openai_test.go
- internal/embedding/voyage.go
- internal/embedding/provider.go
dpage
added a commit
that referenced
this pull request
May 27, 2026
- Use atomic CompareAndSwap for one-time dimension caching to remove the Load+Store race window when concurrent Embed calls complete. - Fail-fast on Embed error in the lazy-dimensions test so a wrapper regression surfaces clearly instead of being hidden. - Update stale comment on tracingRoundTripper: the log level now lives in the chat package, not embedding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
4 tasks
Implements the existing Provider interface against the shared library. NewProvider routes Voyage / OpenAI / Ollama through the wrapper. The old NewProvider is renamed to newProviderOLD in this commit; the old per-provider implementations and the OLD factory are deleted in a later commit of this PR. Dimensions() is lazily populated from the first successful Embed call since the library does not expose vector size up front. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves the LogLevel type and PGEDGE_LLM_LOG_LEVEL env reader into the chat package. Decouples llm_tracing.go from internal/embedding ahead of the embedding package's deletion in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All three providers now go through pgedge-go-llm-lib via the libProvider wrapper. ~1100 lines of provider HTTP code removed; the embedding package now contains only the Provider interface, Config struct, libProvider wrapper, and NewProvider factory. The logger.go file is also deleted; chat package's llm_tracing.go now owns its own log level (moved in the previous commit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Use atomic CompareAndSwap for one-time dimension caching to remove the Load+Store race window when concurrent Embed calls complete. - Fail-fast on Embed error in the lazy-dimensions test so a wrapper regression surfaces clearly instead of being hidden. - Update stale comment on tracingRoundTripper: the log level now lives in the chat package, not embedding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dpage
force-pushed
the
feat/llm-lib-pr2-embeddings
branch
from
7a4cc7f to
2dd224a
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internal/embedding/with a thin wrapper around pgedge-go-llm-lib.embedding.Providerinterface andNewProviderfactory preserved; tool consumers unchanged.internal/chat/llm_tracing.go(added in PR 1) gains a localLogLeveltype so the chat package no longer imports embedding.This is PR 2 of 4. Based on PR 1 (#172). Will need rebase onto main after PR 1 lands.
Notable behaviour change
Provider.Dimensions()now returns 0 until the first successfulEmbedcall (previously hard-coded per known model). The only consumer (generate_embedding) always callsEmbedfirst, so no user-visible regression.Test plan
make testpasses locally.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Documentation
Changed
New Features