[Java] JNI refactor for ONNX Tensor#12281
Conversation
|
@fs-eire @yuslepukhin please can I get a review for this PR? |
Will take a look |
Still need to release OrtStatus In reply to: 1205601429 In reply to: 1205601429 Refers to: java/src/main/native/OrtJniUtil.c:1074 in 68b389f. [](commit_id = 68b389f, deletion_comment = False) |
Documentation question. In reply to: 1205761943 In reply to: 1205761943 Refers to: java/src/main/native/OrtJniUtil.c:1063 in 68b389f. [](commit_id = 68b389f, deletion_comment = False) |
| } | ||
|
|
||
| // Assign the strings into the Tensor | ||
| checkOrtStatus(jniEnv, api, api->FillStringTensor(ortValue, strings, length)); |
There was a problem hiding this comment.
It returns the first exception thrown rather than any later ones.
Java cannot throw multiple exceptions at once, the remainder are discarded. The goal of this refactor is that no other checkOrtStatus calls happen after an exception has been thrown on the Java side, so if there still is one (in this code, I've not finished the OrtSession or OrtJniUtil files) then that's something that needs fixing. |
|
I've fixed all the things discussed. |
|
We are getting troubles from cpplinter. Can you, please, add this change to your PR? |
|
My understanding from #12013 (comment) was that we'd leave the linter as is and accept that it was grumpy about not using C++ casts. But I can add it if you want. |
How about we redo #12094 but only exclude the JNI subfolder? |
| if (code == ORT_OK) { | ||
| // Create the buffers for the Java strings | ||
| const char** strings = NULL; | ||
| code = checkOrtStatus(jniEnv, api, api->AllocatorAlloc(allocator, sizeof(char*) * length, (void**)&strings)); |
There was a problem hiding this comment.
There is no reason to use ORT Allocator here. One can just use malloc or whatever. It would result in a simpler code.
The data is internally copied anyway because C++ code stores it in std::string objects.
There was a problem hiding this comment.
I've been moderately consistently using ORT's allocator as otherwise there are three different memory allocators in use (Java's, malloc and ORT's allocators). I can replace the use of ORT's allocators with malloc if you want, but I'd assumed it was ok as ORT uses it's own allocators to allocate strings in a bunch of the C API methods, rather than malloc and requiring users to free that memory.
There was a problem hiding this comment.
I did not mean to replace all of the cases. Sometimes, the API requires it to be allocated with the ORT allocator. But in this case, it is just a piece of temporary memory which could be on the stack if we could do it.
Ok, I believe I've modified the cpplint action to exclude |
| return NAN; | ||
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| const OrtApi* api = (const OrtApi*) apiHandle; | ||
| if (onnxType == 9) { |
There was a problem hiding this comment.
It is in Java, but these numbers are only in there. As far as I can tell the C enums don't expose the ordinal and I didn't want to have yet another copy of the enum I'd need to keep in sync.
There was a problem hiding this comment.
C enums are ints with values in the order or declaration. But they can be assigned any value you wish.
| jfloat floatVal = convertHalfToFloat(*arr); | ||
| return floatVal; | ||
| } | ||
| } else if (onnxType == 10) { |
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| const OrtApi* api = (const OrtApi*) apiHandle; | ||
| if (onnxType == 1) { |
| return floatVal; | ||
| } | ||
| } else if (onnxType == 10) { | ||
| jfloat* arr = NULL; |
There was a problem hiding this comment.
Yes. All the java types are defined to be a fixed size across platforms.
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| const OrtApi* api = (const OrtApi*) apiHandle; | ||
| if (onnxType == 3) { |
There was a problem hiding this comment.
Same comments as above.
enum + combine the code.
What worries me here, is that in Java a positve uint16_t can become negative.
What would be the best way dealing with it?
I would say we need to return unsigned integers in larger ones. uint16_t needs to be returned in a 32-bit signed.
This may warrant restructuring the API. For example:
We can group types together in a different manner.
uint8_t + int16_t -> jshort
uint16_t + int32_t -> jint
uint32_t + int64_t -> jlong
uint64_t + ????? Some big Java type? #Resolved
There was a problem hiding this comment.
My opinion on that is that people who get unsigned values back in Java should deal with that themselves. The bit pattern is correct, and if you're working in unsigned types you should understand how to sort that out for your use case.
There is no bigger primitive type than long, so we don't have a good solution for that one because you'd end up making an array of objects to hold it and it quickly becomes intractable. When Java gets value types then we'll be able to define unsigned types and return those, but for the moment people usually expect to get the signed versions and if they need something else it's on them to figure it out from the bits.
The cpplint action doesn't pass through the |
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| (void) jobj; // Required JNI parameter not needed by functions which don't need to access their host object. | ||
| const OrtApi* api = (const OrtApi*) apiHandle; | ||
| if (onnxType == 5) { |
|
|
||
| // Get reference to the string | ||
| jobject output = (*jniEnv)->GetObjectArrayElement(jniEnv, outputArray, 0); | ||
| jobjectArray outputArray = createStringArrayFromTensor(jniEnv, api, (OrtAllocator*) allocatorHandle, |
There was a problem hiding this comment.
We check that in Java before this call. The individual "getX" methods are private and only called if it's a single element.
This might be helpful. google/styleguide#220 |
Ah, no I mean the GitHub action which runs cpplint doesn't pass through the exclude argument to the cpplint execution. You can see it complain in one of the runs that exclude is an unsupported option. |
does it work if it is added to "flags"? onnxruntime/.github/workflows/lint.yml Line 89 in e85e31e |
Yep, that works. I'd missed that option the first time around. |
|
/azp run MacOS CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,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 |
|
/azp run orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed onnxruntime-binary-size-checks-ci-pipeline |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run onnxruntime-binary-size-checks-ci-pipeline orttraining-ortmodule-dist |
|
No pipelines are associated with this pull request. |
4 participants
Description:
Following on from #12013, this PR improves the error handling in the JNI code for OnnxTensor. OrtSession and the OrtJniUtil helper file are the remaining bits of unfixed JNI code.
Motivation and Context