[js/rn] support load model from buffer on Android by fs-eire · Pull Request #12676 · microsoft/onnxruntime

fs-eire · 2022-08-23T00:55:16Z

Description: [js/React Native] Add android implementation for creating session from buffer. #12500

skottmckay · 2022-10-11T21:46:03Z

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java

-  public WritableMap loadModel(String uri, InputStream modelStream, ReadableMap options) throws Exception {
-    OrtSession ortSession = null;
-
-    if (!sessionMap.containsKey(uri)) {


What's the reason for removing the check for re-loading the same model?

Is a documentation change required for this as an app that was sloppy about how many times the model was loaded would now get duplicate inference sessions created. Not sure how likely that is in the real world though.

As long as we support loading model from buffer, it is no longer possible to use the model's filepath as key. so I have to remove this check.

There is no documentation for this, and the duplication is never guaranteed - we don't do this for any language bindings for ORT. If users create inference session twice on the same path, we create 2 instances, and this is the expectation.

skottmckay · 2022-10-11T21:47:36Z

js/react_native/lib/backend.ts


-  constructor(path: string) {
+  constructor(pathOrBuffer: string|Uint8Array) {
    this.#inferenceSession = binding;


Do we have tests at the react native level that can be updated to validate this path?

no, but we can add the test case.

still working on getting it tested on the react native level... looks like may require writing a simple react native test app using Jest framework.

https://www.callstack.com/blog/testing-react-native-with-the-new-jest-part-2-redux-snapshots-for-your-actions-and-reducers

skottmckay · 2022-10-15T23:23:48Z

js/react_native/lib/binding.ts

  interface InferenceSession {
    loadModel(modelPath: string, options: SessionOptions): Promise<ModelLoadInfoType>;
+    loadModelFromBase64EncodedBuffer?(buffer: string, options: SessionOptions): Promise<ModelLoadInfoType>;
    run(key: string, feeds: FeedsType, fetches: FetchesType, options: RunOptions): Promise<ReturnType>;


Can we keep the base64 encoding as an implementation detail, and allow the user to provide a Uint8Array instead, maybe renaming this to loadModelFromBytes?

If we replace this binding with a JSI based solution the base64 encoding would be unnecessary.

…fs-eire/js-rn-support-android-load-model-from-buffer

js/react_native/lib/backend.ts

edgchen1 · 2022-11-29T19:47:25Z

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java

+   * @param options onnxruntime session options
+   * @return model loading information map, such as key, input names, and output names
+   */
+  public WritableMap loadModelImpl(String uri, byte[] modelData, ReadableMap options) throws Exception {


this method could be private.
nit: it seems a bit odd to take both uri and modelData parameters in one function. I see that there is some common code after the OrtSession creation, perhaps that can be pulled into a helper function?

yes. it can be pulled into a helper function.

but put it this way as for ios support, we might need to call ort.createSession with both modelPath and model data array - two paths, can be separated this way. just want to match the same pattern there.

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java

edgchen1 · 2022-11-29T20:03:14Z

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java

@@ -46,6 +47,13 @@ public class OnnxruntimeModule extends ReactContextBaseJavaModule {
  private static OrtEnvironment ortEnvironment = OrtEnvironment.getEnvironment();
  private static Map<String, OrtSession> sessionMap = new HashMap<>();


do we ever clean up entries in sessionMap?

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java

This reverts commit 77c97b6.

…13903) ### Description  As title. This pr is missing an un-updated index.android.gradle, which causing an unstable e2e unit test run for React Native CI. Revert the changes for now. ### Motivation and Context  To unblock Ort React Native CI pipeline failure.

**Description**: [js/React Native] Add android implementation for creating session from buffer. microsoft#12500 Co-authored-by: Rachel Guo <guorachel@microsoft.com>

…12676)" (microsoft#13903) ### Description  As title. This pr is missing an un-updated index.android.gradle, which causing an unstable e2e unit test run for React Native CI. Revert the changes for now. ### Motivation and Context  To unblock Ort React Native CI pipeline failure.

### Description  -Add support for loading model from buffer on iOS -Update OnnxruntimeModuleTest to use updated loadModelFromBuffer Based on #12676 ### Motivation and Context  Issue: #12500 --------- Co-authored-by: rachguo <rachguo@rachguos-Mini.attlocal.net> Co-authored-by: rachguo <rachguo@rachguos-Mac-mini.local>

### Description  -Add support for loading model from buffer on iOS -Update OnnxruntimeModuleTest to use updated loadModelFromBuffer Based on microsoft#12676 ### Motivation and Context  Issue: microsoft#12500 --------- Co-authored-by: rachguo <rachguo@rachguos-Mini.attlocal.net> Co-authored-by: rachguo <rachguo@rachguos-Mac-mini.local>

**Description**: [js/React Native] Add android implementation for creating session from buffer. microsoft#12500 Co-authored-by: Rachel Guo <guorachel@microsoft.com>

…12676)" (microsoft#13903) ### Description  As title. This pr is missing an un-updated index.android.gradle, which causing an unstable e2e unit test run for React Native CI. Revert the changes for now. ### Motivation and Context  To unblock Ort React Native CI pipeline failure.

### Description  -Add support for loading model from buffer on iOS -Update OnnxruntimeModuleTest to use updated loadModelFromBuffer Based on microsoft#12676 ### Motivation and Context  Issue: microsoft#12500 --------- Co-authored-by: rachguo <rachguo@rachguos-Mini.attlocal.net> Co-authored-by: rachguo <rachguo@rachguos-Mac-mini.local>

fs-eire added 2 commits August 22, 2022 17:53

[js/rn] support load model from buffer on Android

49d26d6

update tests

e9d4c47

skottmckay reviewed Oct 11, 2022

View reviewed changes

skottmckay reviewed Oct 15, 2022

View reviewed changes

edgchen1 mentioned this pull request Oct 19, 2022

React Native .ort Operator Issue #12743

Closed

YUNQIUGUO added 2 commits November 16, 2022 18:35

pass in bytearray for react native loadmodel binding

3c7855a

Merge branch 'main' of https://github.com/microsoft/onnxruntime into …

8ee3a23

…fs-eire/js-rn-support-android-load-model-from-buffer

github-advanced-security bot found potential problems Nov 17, 2022

View reviewed changes

js/react_native/lib/backend.ts Fixed Show fixed Hide fixed

YUNQIUGUO added 4 commits November 16, 2022 18:46

fix

fcedd5b

update

efae747

passing in readablearray

3356325

revert to use base64encodedstring in bindings and some refactor

bf22801

edgchen1 reviewed Nov 29, 2022

View reviewed changes

YUNQIUGUO added 3 commits November 29, 2022 13:59

update more texts pr comments

ff288a9

update

5a11621

fix biginteger change

6748f26

edgchen1 reviewed Nov 30, 2022

View reviewed changes

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java Outdated Show resolved Hide resolved

commit pr comment

fc0ceb7

edgchen1 reviewed Nov 30, 2022

View reviewed changes

js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java Outdated Show resolved Hide resolved

minor update

a339a41

edgchen1 approved these changes Nov 30, 2022

View reviewed changes

YUNQIUGUO merged commit 77c97b6 into main Nov 30, 2022

YUNQIUGUO deleted the fs-eire/js-rn-support-android-load-model-from-buffer branch November 30, 2022 18:55

YUNQIUGUO mentioned this pull request Dec 1, 2022

[rn] Add support for loading model from buffer on iOS #13802

Merged

YUNQIUGUO added a commit that referenced this pull request Dec 8, 2022

Revert "[js/rn] support load model from buffer on Android (#12676)"

04bd01c

This reverts commit 77c97b6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[js/rn] support load model from buffer on Android#12676

[js/rn] support load model from buffer on Android#12676
YUNQIUGUO merged 13 commits intomainfrom
fs-eire/js-rn-support-android-load-model-from-buffer

fs-eire commented Aug 23, 2022

Uh oh!

skottmckay Oct 11, 2022

Uh oh!

fs-eire Oct 12, 2022

Uh oh!

skottmckay Oct 11, 2022

Uh oh!

fs-eire Oct 12, 2022

Uh oh!

YUNQIUGUO Nov 17, 2022

Uh oh!

skottmckay Oct 15, 2022

Uh oh!

Uh oh!

edgchen1 Nov 29, 2022

Uh oh!

YUNQIUGUO Nov 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edgchen1 Nov 29, 2022

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -46,6 +47,13 @@ public class OnnxruntimeModule extends ReactContextBaseJavaModule {
		private static OrtEnvironment ortEnvironment = OrtEnvironment.getEnvironment();
		private static Map<String, OrtSession> sessionMap = new HashMap<>();

Conversation

fs-eire commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants