Skip to content

fix(llm): preserve semantic embedding batch order - #377

Merged
imbajin merged 1 commit into
apache:mainfrom
JesusMan0529:fix/semantic-embedding-order
Oct 3, 2026
Merged

imbajin merged 1 commit into
apache:mainfrom
JesusMan0529:fix/semantic-embedding-order

Conversation

@JesusMan0529

Copy link
Copy Markdown
Contributor

When more than 1,000 new vertices are indexed, embedding batches can finish out of order. BuildSemanticIndex currently flattens those results in completion order but pairs them with the original vertex IDs, so semantic search can return the wrong vertex.

Collect batch results with asyncio.gather to retain input order. Keep the existing batch size, concurrency limit, synchronous embedding provider calls, and progress updates as each batch completes.

The regression forces the trailing batch to finish before the first batch and checks real Faiss save/load/search for both PRIMARY_KEY and CUSTOMIZE IDs. It fails on the original code. Empty input and provider error propagation are also covered.

Validation (Python 3.11):

  • SKIP_EXTERNAL_SERVICES=true uv run pytest hugegraph-llm/src/tests/operators/index_op/ hugegraph-llm/src/tests/indices/ -q --tb=short: 53 passed, 1 skipped (the existing live Ollama test).
  • uv run ruff format --check .: passed.
  • uv run ruff check .: passed.
  • git diff --check: passed.

HugeGraph Server and live LLM/provider integration tests were not run locally.

Copilot AI balanced review requested due to automatic review settings October 3, 2026 03:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the llm label Oct 3, 2026
@JesusMan0529

Copy link
Copy Markdown
Contributor Author

The regression uses 1,001 vertices and forces the one-vertex trailing batch to complete before the first 1,000-vertex batch. It checks nearest-neighbor lookup after saving and reloading a real Faiss index, for both PRIMARY_KEY and CUSTOMIZE ID strategies; it fails on the original operator and passes with this change. Batch completion still advances the progress bar immediately, and empty input and provider errors are covered. The related index-operator/Faiss suite passed with 53 tests and one existing Ollama-service test skipped.

@JesusMan0529

Copy link
Copy Markdown
Contributor Author

Hope it could be merged 😊

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx

@imbajin
imbajin merged commit 9922568 into apache:main Oct 3, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants