rzo1 commented on code in PR #1462:
URL: https://github.com/apache/opennlp/pull/1462#discussion_r4259276173
##########
opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/AbstractDL.java:
##########
@@ -216,17 +217,44 @@ protected AbstractDL(final OrtEnvironment env, final
OrtSession session,
* @param inferenceOptions The inference options to read the GPU
configuration from.
* @return The configured session options.
*
- * @throws OrtException Thrown if the CUDA execution provider cannot be
added.
+ * @throws OrtException Thrown if the CUDA execution provider cannot be
added: the ONNX Runtime
+ * on the classpath has no CUDA support, its CUDA libraries cannot be
loaded, or no device
+ * answers to the configured device id. ONNX Runtime does not fall back
to the CPU. The
+ * session options created for the request are closed before the
exception is thrown.
* @throws IllegalArgumentException Thrown if {@code inferenceOptions} is
{@code null} or its
* split sizes are invalid, see {@link #validateSplitOptions(int, int)}.
*/
protected static OrtSession.SessionOptions sessionOptions(final
InferenceOptions inferenceOptions)
throws OrtException {
+ return sessionOptions(inferenceOptions, OrtSession.SessionOptions::new);
+ }
+
+ /**
+ * Builds ONNX session options as {@link #sessionOptions(InferenceOptions)}
does, from options
+ * that {@code factory} creates.
+ *
+ * @param inferenceOptions The inference options to read the GPU
configuration from.
+ * @param factory Creates the session options to configure.
+ * @return The configured session options.
+ *
+ * @throws OrtException Thrown if the CUDA execution provider cannot be
added.
+ * @throws IllegalArgumentException Thrown if {@code inferenceOptions} is
{@code null} or its
+ * split sizes are invalid.
+ */
+ static OrtSession.SessionOptions sessionOptions(final InferenceOptions
inferenceOptions,
+ final Supplier<OrtSession.SessionOptions> factory) throws OrtException {
ParamChecks.requireNonNullArg(inferenceOptions, "inferenceOptions");
validateSplitOptions(inferenceOptions);
- final OrtSession.SessionOptions sessionOptions = new
OrtSession.SessionOptions();
+ final OrtSession.SessionOptions sessionOptions = factory.get();
if (inferenceOptions.isGpu()) {
- sessionOptions.addCUDA(inferenceOptions.getGpuDeviceId());
+ try {
+ // addCUDA needs ONNX Runtime's default logger, which exists once an
environment does.
+ OrtEnvironment.getEnvironment();
+ sessionOptions.addCUDA(inferenceOptions.getGpuDeviceId());
Review Comment:
A negative device id reaches `addCUDA` unchecked and fails as `OrtException`
(pinned in `SentenceVectorsDLInferenceOptionsTest`). Please reject it with
`ParamChecks.requireNonNegative` before this call, add the `@throws`, and move
`-1` to an IAE case in the test.
##########
opennlp-core/opennlp-ml/opennlp-dl/src/test/java/opennlp/dl/vectors/SentenceVectorsDLEmbedderTest.java:
##########
@@ -60,7 +63,6 @@
*/
class SentenceVectorsDLEmbedderTest {
Review Comment:
**Nit:** double blank line after removing `DELTA`.
##########
opennlp-core/opennlp-ml/opennlp-dl/src/main/java/opennlp/dl/AbstractDL.java:
##########
@@ -216,17 +217,44 @@ protected AbstractDL(final OrtEnvironment env, final
OrtSession session,
* @param inferenceOptions The inference options to read the GPU
configuration from.
* @return The configured session options.
*
- * @throws OrtException Thrown if the CUDA execution provider cannot be
added.
+ * @throws OrtException Thrown if the CUDA execution provider cannot be
added: the ONNX Runtime
+ * on the classpath has no CUDA support, its CUDA libraries cannot be
loaded, or no device
+ * answers to the configured device id. ONNX Runtime does not fall back
to the CPU. The
+ * session options created for the request are closed before the
exception is thrown.
* @throws IllegalArgumentException Thrown if {@code inferenceOptions} is
{@code null} or its
* split sizes are invalid, see {@link #validateSplitOptions(int, int)}.
*/
protected static OrtSession.SessionOptions sessionOptions(final
InferenceOptions inferenceOptions)
throws OrtException {
+ return sessionOptions(inferenceOptions, OrtSession.SessionOptions::new);
+ }
+
+ /**
+ * Builds ONNX session options as {@link #sessionOptions(InferenceOptions)}
does, from options
+ * that {@code factory} creates.
+ *
+ * @param inferenceOptions The inference options to read the GPU
configuration from.
+ * @param factory Creates the session options to configure.
+ * @return The configured session options.
+ *
+ * @throws OrtException Thrown if the CUDA execution provider cannot be
added.
+ * @throws IllegalArgumentException Thrown if {@code inferenceOptions} is
{@code null} or its
+ * split sizes are invalid.
+ */
+ static OrtSession.SessionOptions sessionOptions(final InferenceOptions
inferenceOptions,
Review Comment:
This static overload with a `Supplier` exists only for `GpuRequestTest` to
observe `close()`. Please inline it into `sessionOptions(InferenceOptions)`; I
would rather not have a test seam in production code.
##########
opennlp-core/opennlp-ml/opennlp-dl-gpu/pom.xml:
##########
@@ -102,5 +109,32 @@
</plugins>
</build>
+ <!-- onnxruntime_gpu 1.30.0 ships native libraries for linux-x64 and win-x64
only, unlike the
Review Comment:
Main is on ONNX Runtime 1.31.0 now (#1491). Please rebase and drop the
version from the comment, so it does not go stale on the next bump.
##########
opennlp-docs/src/docbkx/machine-learning.xml:
##########
@@ -182,5 +182,25 @@ try (TextEmbedder embedder =
providers.select(spec).create(spec)) {
until another one is installed. Existing
<code>SentenceVectorsDL</code> constructors remain
available.
</para>
+ <para>
+ To compute the vectors on a GPU, depend on
<code>opennlp-dl-gpu</code> in place of
+ <code>opennlp-dl</code> and pass
<code>InferenceOptions</code> to
+ <code>SentenceVectorsDL</code>.
<code>opennlp-dl-gpu</code> supports linux-x64 and
+ win-x64 only, and the machine needs the CUDA and cuDNN
versions that the ONNX Runtime
+ release requires, see the
+ <ulink
url="https://onnxruntime.ai/docs/execution-providers/CUDA-ExecutionProvider.html#requirements";>CUDA
+ execution provider requirements</ulink>. The
<code>onnx</code> provider spec has no device
+ option yet, so an embedder created through it runs on
the CPU. If CUDA or the configured device is not available, the
Review Comment:
**Nit:** please rewrap to the surrounding width and write "has no device
option" (no "yet" in user docs).
##########
opennlp-core/opennlp-ml/opennlp-dl/src/test/java/opennlp/dl/GpuRequestTest.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package opennlp.dl;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
+
+import ai.onnxruntime.OrtException;
+import ai.onnxruntime.OrtSession;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import opennlp.dl.doccat.DocumentCategorizerDL;
+import opennlp.dl.doccat.scoring.AverageClassificationScoringStrategy;
+import opennlp.dl.namefinder.NameFinderDL;
+import opennlp.tools.sentdetect.NewlineSentenceDetector;
+
+import static opennlp.dl.OnnxTestSupport.UNUSABLE_DEVICE_ID;
+import static opennlp.dl.OnnxTestSupport.copy;
+import static opennlp.dl.OnnxTestSupport.gpu;
+import static opennlp.dl.vectors.TinyVectorModels.TINY_VECTORS;
+import static opennlp.dl.vectors.TinyVectorModels.model;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.abort;
+
+/**
+ * Tests how a GPU request in {@link InferenceOptions} reaches ONNX Runtime: a
request for a
+ * device that is not usable is rejected with an {@link OrtException}, never
served by the CPU,
+ * and a CPU request does not ask for any device.
+ *
+ * <p>In {@code opennlp-dl}, whose ONNX Runtime has no CUDA support, every GPU
request is
+ * rejected, so the tests there check that a request never falls back to the
CPU. The rejection
+ * of the device id itself is checked when {@code opennlp-dl-gpu} runs these
tests.</p>
+ */
+class GpuRequestTest {
+
+ private static final String PLAIN_VOCAB = "/opennlp/dl/vocab-plain.txt";
+
+ /** The device id is not read while {@code gpu} is off, so no provider is
requested. */
+ @ParameterizedTest
+ @ValueSource(ints = {-1, 0, UNUSABLE_DEVICE_ID})
+ void testSessionOptionsWithoutGpuIgnoreTheDeviceId(final int deviceId) {
+ final InferenceOptions options = new InferenceOptions();
+ options.setGpuDeviceId(deviceId);
+ assertDoesNotThrow(() -> {
+ try (OrtSession.SessionOptions sessionOptions =
AbstractDL.sessionOptions(options)) {
+ assertNotNull(sessionOptions);
+ }
+ });
+ }
+
+ /** A name finder on a device id that no machine has fails instead of
running on the CPU. */
+ @Test
+ void testNameFinderRejectsAnUnusableDevice(@TempDir final Path dir) throws
IOException {
+ final File model = model(dir, TINY_VECTORS);
+ final File vocab = copy(dir, PLAIN_VOCAB, "vocab.txt");
Review Comment:
**Nit:** `"vocab.txt"` is repeated (also line 93); a constant next to
`PLAIN_VOCAB`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
