Skip to content

fix(embedder): route batch embedding to /v1/embeddings (Issue #629)#631

Open
jlin53882 wants to merge 3 commits intoCortexReach:masterfrom
jlin53882:fix/issue-629-clean
Open

fix(embedder): route batch embedding to /v1/embeddings (Issue #629)#631
jlin53882 wants to merge 3 commits intoCortexReach:masterfrom
jlin53882:fix/issue-629-clean

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

Summary

Fix Issue #629: Ollama batch embedding fails after PR #621

Problem

PR #621 fixed Ollama 0.20.5+ single embedding by switching to /api/embeddings with prompt field. However, batch requests send payload.input as string[], causing Ollama to reject with:

"json: cannot unmarshal array into Go struct field EmbeddingRequest.prompt of type string"

Solution

Route requests by input type inside embedWithNativeFetch():

Input Type Endpoint Field
Single (string) /api/embeddings prompt (PR #621 unchanged)
Batch (string[]) /v1/embeddings input

Additionally, add response validation to ensure batch responses contain the expected count and non-empty embeddings.

Changes

  • src/embedder.ts: Route based on Array.isArray(payload.input)
  • test/embedder-ollama-batch-routing.test.mjs: 5 test cases covering routing and validation

Testing

Test Result
Single /api/embeddings with prompt PASS
Batch /v1/embeddings with input[] PASS
Batch count mismatch rejection PASS
Batch empty embedding rejection PASS
Single-element batch routing PASS

Limitation

This assumes /v1/embeddings batch mode is valid for the target Ollama/model combination. Verified with jina-v5-retrieval-test.

Closes #629

Fix Issue CortexReach#629: Ollama batch embedding fails after PR CortexReach#621

PR CortexReach#621 fixed single embedding by switching to /api/embeddings with prompt
field. However, batch requests send payload.input as string[], which
causes Ollama to reject with 'cannot unmarshal array into prompt'.

Changes:
- Route single input (string) to /api/embeddings + prompt (PR CortexReach#621)
- Route batch input (string[]) to /v1/embeddings + input[]
- Add response validation for count and non-empty embeddings
- Add test/embedder-ollama-batch-routing.test.mjs with 5 test cases

Testing:
- Local testing with jina-v5-retrieval-test confirms both paths work
- All 5 new tests pass

Closes CortexReach#629
@jlin53882 jlin53882 force-pushed the fix/issue-629-clean branch from 7b8a0f8 to d2e68a6 Compare April 15, 2026 18:12
@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Apr 16, 2026

Clean fix for the batch embedding regression introduced by #621. The root cause diagnosis is correct and the routing split is the right approach. Ready to merge with a few minor notes.

Nice to have (non-blocking)

  • Tests bind to port 11434 — the live Ollama default. Any developer running Ollama locally will hit EADDRINUSE. Consider a random available port or mock the HTTP layer instead.
  • The two positive routing tests assert the response shape but not which endpoint was called — a future routing regression won't be caught. Worth asserting the request URL in at least one test.
  • The new batch native-fetch branch discards provider options already computed by buildPayload() — double-check whether timeout, headers, or other options from that path are needed on the batch route.
  • /v1/embeddings is used unconditionally for batch with no fallback — if a model doesn't support that endpoint, the failure will be silent from the user's perspective. A comment noting the assumption would help future maintainers.

CI note: New test file test/embedder-ollama-batch-routing.test.mjs appears unregistered in the CI test manifest — verify-ci-test-manifest.mjs may need updating before the full suite passes.

Before merge: stale_base=true — quick rebase against main to confirm nothing has touched embedder.ts there.

- Add embedder-ollama-batch-routing.test.mjs to CI manifest
- Add comments explaining why provider options are omitted for Ollama batch
- Add note about /v1/embeddings no-fallback assumption

Reviewed by: rwmjhb
- Add embedder-ollama-batch-routing.test.mjs to CI manifest
- Add comments explaining why provider options are omitted for Ollama batch
- Add note about /v1/embeddings no-fallback assumption

Reviewed by: rwmjhb
@jlin53882
Copy link
Copy Markdown
Contributor Author

PR Review Updates

All review comments from @rwmjhb have been addressed:

Fixed:

  1. CI manifest - Added test/embedder-ollama-batch-routing.test.mjs to scripts/ci-test-manifest.mjs
  2. Port conflict - Changed test to use port 0 (OS-assigned available port) instead of hardcoded 11434, avoiding EADDRINUSE when developers have Ollama running locally
  3. Endpoint verification - Test already validates which endpoint is called via capturedRoute assertion
  4. Provider options - Added code comments explaining why encoding_format, normalized, etc. are omitted for Ollama batch (Ollama doesn't support these params)
  5. No fallback - Added comment documenting the assumption that /v1/embeddings is universally supported

Rebase Check

Verified: embedder.ts has not changed in upstream master since PR base (16ee3e4). No rebase needed.

Ready for merge! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Ollama batch embedding fails after PR #621 (array input rejected)

2 participants