Repository navigation
Conversation
krickert
force-pushed
the
feat/GH-1391-embed-batch-cap
branch
2 times, most recently
from
October 11, 2026 12:20
82156b0 to
dc68fd7
Compare
krickert
marked this pull request as ready for review
October 11, 2026 16:17
rzo1
requested changes
Oct 11, 2026
rzo1
left a comment
Contributor
There was a problem hiding this comment.
Hi @krickert, thanks for the PR. The splitting in batches(...) looks correct to me. Some remarks inline, and two on the process:
- This PR contains the commit of #1462, which currently has changes requested. Please mark it as draft until #1462 is merged and rebase it afterwards; the review points on the shared test fixtures of #1462 apply here as well.
- The description mentions new tests in
SentenceVectorsDLEmbedderTest, but the commit only addsSentenceVectorsDLBatchLimitTest. Please also set the milestone and labels.
No eval build is needed, since no model and no default behaviour changes.
krickert
marked this pull request as draft
October 11, 2026 20:07
krickert
force-pushed
the
feat/GH-1391-embed-batch-cap
branch
3 times, most recently
from
October 13, 2026 19:36
e540ab2 to
8517809
Compare
Contributor
Author
…t CUDA request in a fresh JVM SentenceVectorsDL always created default session options, so it ran on the CPU even with opennlp-dl-gpu. A new constructor takes InferenceOptions, like NameFinderDL and DocumentCategorizerDL, and uses the GPU settings and the lower case setting from them; a lower case setting in the options takes precedence over the lowerCase argument. The existing constructors keep their behavior. AbstractDL.sessionOptions added the CUDA provider before any OrtEnvironment existed. Loading the provider needs ONNX Runtime's default logger, which only exists once an environment does, so the first GPU request in a fresh JVM failed with "Attempt to use DefaultLogger but none has been registered". The environment is now created first. This affects NameFinderDL and DocumentCategorizerDL as well. When the provider cannot be added, the session options created for the request are closed before the exception is thrown. A test makes the first CUDA request of a child JVM and requires that the provider is added or rejected for a reason other than the missing default logger; it runs in opennlp-dl-gpu and fails with the AbstractDL of main. Other tests request a device id no machine has and check that ONNX Runtime rejects it for SentenceVectorsDL, NameFinderDL and DocumentCategorizerDL, and compare vectors on CUDA device 0 with the CPU session (skipped without a CUDA provider or device). The tiny model test fixtures are shared in TinyVectorModels. The opennlp-dl-gpu test classpath no longer has the CPU onnxruntime next to onnxruntime_gpu. Since onnxruntime_gpu has native libraries for linux-x64 and win-x64 only, the opennlp-dl-gpu tests are skipped on macOS and aarch64. The machine learning chapter documents the GPU setup and the supported platforms. Fixes #1389 Fixes #1390 Co-authored-by: Richard Zowalla <rzo1@apache.org> Co-authored-by: Martin Wiesner <mawiesne@apache.org> Signed-off-by: Kristian Rickert <kristian@apache.org> Co-authored-by: Claude Code 2.1.296 (Opus 5.5) <noreply@anthropic.com>
…ference TextEmbedder.embedAll and the manual say the implementation forms its own batches, so a caller may pass a list of any size. SentenceVectorsDL grouped the texts by tokenized length and ran each group as one inference, however many texts it had. Each group is now split so that no inference takes more than 16384 token positions (rows times row length), 32 rows at the BERT length of 512. An input longer than that runs in a batch of its own. Groups run in the order their length first appears in the call. Vectors do not change: batches are not padded, so every row is computed from the same tensors as its single-input call. The grouping moved into a package-private batches method so tests can check the inference plan. SentenceVectorsDLBatchLimitTest covers row count boundaries (exact fit, one over, one under, lengths that do and do not divide the cap), a length at and over the cap, an empty call, a mix where only one group splits, input order with alternating lengths, CLS and mean pooling, the pooled-output model and unit length scaling through a split group. The GPU parity test also runs a split group, and the tiny model texts and expected vectors are shared in TinyVectorModels. Fixes #1391 Signed-off-by: Kristian Rickert <kristian@apache.org> Co-authored-by: Claude Code 2.1.296 (Opus 5.5) <noreply@anthropic.com>
krickert
force-pushed
the
feat/GH-1391-embed-batch-cap
branch
from
October 15, 2026 13:29
8517809 to
72f83f4
Compare
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
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.
Draft until #1462 is merged
TextEmbedder.embedAll and the manual say the implementation forms its own batches, so a caller may pass a list of any size. SentenceVectorsDL.embedAll groups texts by tokenized length and splits each group so that no inference takes more than 16384 token positions (rows times row length), which is 32 rows at the BERT length of 512. An input longer than 16384 tokens runs in a batch of its own.
Vectors do not change: batches are not padded, so every row is computed from the same tensors as its single-input call.
The grouping is in a package-private batches(Tokens[]) that returns the index lists embedAll runs, so the tests can check the inference plan directly. MAX_BATCH_TOKEN_POSITIONS is a package-private constant, not an option.
AI assisted test description:
Fixes #1391