Skip to content

feat: support batch embedding requests.#1828

Open
DongheJin wants to merge 1 commit into
xLLM-AI:mainfrom
DongheJin:feat/embedding
Open

feat: support batch embedding requests.#1828
DongheJin wants to merge 1 commit into
xLLM-AI:mainfrom
DongheJin:feat/embedding

Conversation

@DongheJin

Copy link
Copy Markdown
Collaborator

Description

Related Issues

Change Type

  • Bug fix
  • New feature
  • Performance improvement
  • Refactor
  • Documentation
  • Test
  • Build or CI

Pull Request Checklist

Thank you for contributing to xLLM. Before requesting review, please make sure the following items are complete.

PR Title and Commit Messages

  • The PR title and each commit message follow the xLLM commit format: <type>: <subject>.

Allowed types: feat, bugfix, docs, test, refactor, chore, style, revert, perf, model, build, release.
The subject should use clear English, start with a verb, include at least 4 words, and end with ..

Pre-commit Checks

  • I have installed pre-commit by running pip install pre-commit or an equivalent command.
  • I have installed the hooks with pre-commit install.
  • I have run pre-commit run --all-files and fixed any reported issues.

If you are unsure how to set up pre-commit, see the pre-commit documentation.

Self Review

  • I have self-reviewed the code according to .agents/skills/code-review/references/custom-code-style.md, especially code written or assisted by AI.
  • I have rebased this PR onto the latest main branch.

Build and Test Coverage

  • Tests have been added or updated as needed.
  • CUDA: python setup.py build test has passed on a CUDA machine.
  • NPU: python setup.py build test has passed on an NPU machine.
  • MLU: python setup.py build test has passed on an MLU machine.

Reviewer Notes

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces support for batch text embedding requests by adding handle_text_embedding_request in api_service.cpp and implementing BatchEmbeddingContext and process_batch_async in embedding_service_impl.cpp. Feedback highlights two critical issues: first, a potential crash when calling contains() on a JSON body that is not an object, which can be resolved by checking is_object() first; second, duplicate request_ids in batch requests that could cause scheduler collisions, which should be resolved by appending the batch index to each request ID.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.


std::vector<std::string> inputs;
bool is_batch = false;
if (json_body.contains("input")) {

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.

critical

Calling contains() on a nlohmann::json object when it is not of object type (e.g., if the client sends a JSON array, null, or primitive value) will throw a nlohmann::json::type_error exception, causing the request thread to crash or abort. Check json_body.is_object() first to ensure safe access.

Suggested change
if (json_body.contains("input")) {
if (json_body.is_object() && json_body.contains("input")) {

int64_t created_time = absl::ToUnixSeconds(absl::Now());

size_t num_requests = inputs.size();
std::vector<RequestParams> sps(num_requests, request_params);

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.

critical

Copying request_params directly for all requests in the batch results in duplicate request_ids. In the engine's scheduler, request_id must be unique to prevent collisions, state overwrites, or crashes. Appending the batch index to the base request_id ensures uniqueness while maintaining traceability. This also adheres to the style guide by using reserve() and emplace_back().

  std::vector<RequestParams> sps;
  sps.reserve(num_requests);
  for (size_t i = 0; i < num_requests; ++i) {
    RequestParams sp = request_params;
    sp.request_id = request_id + "-" + std::to_string(i);
    sps.emplace_back(std::move(sp));
  }
References
  1. Always reserve() before filling a std::vector when the size is known or can be estimated, and prefer emplace_back over push_back. (link)

@DongheJin DongheJin changed the title fix: support batch embedding requests fix: support batch embedding requests. Jun 26, 2026
@DongheJin DongheJin changed the title fix: support batch embedding requests. feat: support batch embedding requests. Jun 26, 2026
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.

2 participants