[java] Changes OrtEnvironment so it can't be closed by users#10670
[java] Changes OrtEnvironment so it can't be closed by users#10670pranavsharma merged 2 commits intomicrosoft:masterfrom
Conversation
| env.close(); | ||
| assertTrue(env.isClosed()); | ||
| } | ||
| private static final OrtEnvironment env = OrtEnvironment.getEnvironment(); |
There was a problem hiding this comment.
No, I've not added an explicit test for that. I can do.
|
/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux CPU x64 NoContribops CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline |
|
/azp run MacOS NoContribops CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed |
|
Azure Pipelines successfully started running 7 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 7 pipeline(s). |
|
FYI see my comment here #10200 (comment). Could close() offer the same functionality that the C API offers? I suppose that would help with the tests of creating the env with different options? |
|
The close is the problem though, my understanding is that closing the We could refcount it (as it was before) but the try-with-resources idiom that is encouraged in Java would mean that it's being closed and reopened multiple times. There's no good way for a user to signal that they are completely done with an object such that it can't be rebuilt ever again in Java, because most of the time that's not a useful thing to do. |
|
/azp run onnxruntime-binary-size-checks-ci-pipeline, onnxruntime-python-checks-ci-pipeline, ONNX Runtime Web CI Pipeline |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Craigacp
force-pushed
the
java-environment-refactor
branch
from
7c8f09b to
b37aee6
Compare
|
Rebased after the merge of #10318. |
|
/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline |
|
/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline |
|
Azure Pipelines successfully started running 8 pipeline(s). |
|
Azure Pipelines successfully started running 9 pipeline(s). |
pranavsharma
left a comment
pranavsharma
left a comment
There was a problem hiding this comment.
nit: unrelated to the objective of this PR - the check for INSTANCE == null can be avoided in the getEnvironment variants keeping it in only once place.
The three checks do different things. The one in |
|
/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline |
|
/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline |
|
Azure Pipelines successfully started running 8 pipeline(s). |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
LGTM 👍 |
|
Thanks for the change @Craigacp! |
4 participants
Description:
The changes in #10200 enforce that OrtEnv can't be created more than once in a single process. The Java API used to allow this to ensure that users could configure thread pools and logging levels in the environment. This PR changes
OrtEnvironmentso there can only be a single instance of it for the lifetime of the JVM (well, as much as it can be enforced in Java, it may be possible to unload theOrtEnvironment.classentirely and reload it, resetting the state of its statics and allowing another environment to be created). The environment now registers a JVM shutdown hook which releases the environment as the JVM shuts down. This could cause the environment to be released while a daemon thread which uses ORT is running, as daemon threads concurrently execute with the shutdown hooks, however it's not possible to prevent this from within Java.This caused a few refactorings in the tests to ensure that the thread pool tests can still run. These refactorings slow down the Java tests slightly as they now need to fork a new JVM for each test class. There might be some implication for the android build system here if it runs the Java tests as part of the build, but I'm not familiar with how those are used.
OrtEnvironmentstill implementsAutoCloseablethough the close method is now a no-op. Removing this interface would be a breaking change on users, and we can consider if we want to do that or not.Much of this PR is visual noise in the tests from the indentation change of moving the environment creation out of a try-with-resources block.
Motivation and Context