Repository navigation
Conversation
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Hi @krickert - thanks, good to have this. One point should be aligned before merging: the AI disclosure rules need to follow the ASF guidance.
- The ASF generative tooling guidance recommends a
Generated-by: <Name of Tool> <Version>token in the commit message, and the RAI commit-message practice asks to name the actual tool and model rather than a generic label. The current "Do not add model identifiers" and theAssisted-by:trailer contradict both. Suggested wording:Disclose AI help in the commit message with a
Generated-by: <tool> <version>trailer, naming the actual tool and model version, as recommended by the ASF generative tooling guidance. - Please reference the ASF page as the primary policy; the Lucene AI policy can stay as an additional reference.
- Minor: "disucssion" → "discussion"; "enforce the user to post to discussions on GITHUB" → "ask on GitHub Discussions or dev@opennlp.apache.org"; "Standard JDK character and space tools are usually rewritten in OpenNLP" duplicates the following two bullets and can be removed.
WDYT?
There was a problem hiding this comment.
🟡 Changes recommended
The policy differs from the referenced Lucene policy, omits the requested repository layout, and contains unclear instructions.
5 open findings
What changed in this PR
Adds repository-wide guidance for AI-assisted OpenNLP development.
Changes:
- Defines AI contribution policy.
- Documents build, coding, testing, and documentation standards.
- Establishes commit and dependency rules.
| File | Description |
|---|---|
AGENTS.md |
Adds guidance for AI coding agents and contributors. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - JDK 21 and `./mvnw`. With `-pl`, add `-am`. | ||
| - Run `./mvnw verify` on the touched modules before proposing a change; | ||
| checkstyle and forbiddenapis run there. | ||
| - Do not introduce new dependencies without a disucssion and approval |
I agree to show the tool, the version of the model and version of the harness. We agreed to follow the lucene standard too and had some standards in the email mentioned- totally agree. For the most part (pardon my french) "don't be a jerk" is the overall arching theme - but since it's an LLM, that's pretty subjective. I think all of the suggestions are spot-on - it's such an important piece that I expect a couple rounds of discussion on this one. I'm more of an AB test sorta guy, given the uncertainty of LLMs, I think whatever our first version is we'll find tweaks we need to do all the time. A year from now let's hope the code produced never looks mean to us humans. When I first used AI, I used to say "it must compile before I look at it" or "look at the source code instead of stack overflow" - that is no longer needed. |
AFAIR we talked about it on the dev@ list but there was no vote nor consensus found yet ;-) |
Agreed :) It's good we're doing it now because putting something out there is the best way to get the vote out. I'll make all the changes in a few moments. But my vote - Version ID of the harness, model/version, and manual sign-off. |
1f4dc58 to
f976894
Compare
|
Thanks, did everything we discussed. The file follows the ASF generative tooling guidance as its only policy reference. The disclosure line calls for a Tomorrow (or tonight) I'll start a [DISCUSS] thread on dev@ so the list can settle the wording. |
|
Also - I kept the removal of the lucene ref (ASF is mentioned though). No need to have it in AGENTS.md - it will confuse the LLMs when there's contradictions multiple versions etc... I'll continue our DEV thread. |
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Hi @krickert, thanks for the update; the structure and the eval section are good. Some remarks:
- Since this file sets contribution policy, I would merge it only after we reached consensus on dev@ (there was no vote yet).
- Please add a concrete example of the trailer, e.g.
Co-authored-by: <tool> <version> (<model> <version>) <email>. GitHub only parsesCo-authored-bywith an e-mail address, and a fixed format keeps the disclosure machine-readable as the ASF guidance intends. - The regex rule should mention the exception from CONTRIBUTING.md (user-supplied patterns as public contract, marked with
@SuppressForbidden), or simply reference CONTRIBUTING.md, so the two files do not diverge.
…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)
ea76e60 to
6390110
Compare
…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)
…rs off the heap SentenceVectorsDLBenchmark times embed per text and embedAll for calls of 1 to 128 texts of mixed length, on the CPU or CUDA device 0. The opennlp-dl module gets the same jmh profile as opennlp-runtime, and BENCHMARKS.md documents how to run it and the allocation numbers. Each inference copied the inputs from long arrays into heap buffers, which ONNX Runtime copied again into direct memory, and fetched the output through getValue(), which builds one float array per token. The inputs are now written into direct buffers, and ONNX Runtime writes the output into a direct buffer that pooling uses in place. Only the requested output is computed. SentenceVectorsDLBenchmark with -prof gc on all-MiniLM-L6-v2: heap allocation per call is 58 to 69 percent lower on CPU and GPU, for example embedAll of 128 texts on the GPU drops from 7904 KB to 2424 KB. Time per call on the GPU is unchanged. Signed-off-by: Kristian Rickert <kristian@apache.org> Co-authored-by: Claude Code 2.1.296 (Opus 5.5)
6390110 to
785ad81
Compare
Adds AGENTS.md at the repository root: short rules for AI coding agents and the people who run them. It covers the AI policy (the committer reviews and owns AI output; agents do not post comments, issues or PRs on their own, following the Lucene AI policy), the build, the code conventions the project enforces (no regex in production code, existing helpers before new ones, StringUtil for case and whitespace, ParamChecks), Javadoc, tests and commit format.
Fixes #1453