Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a react-native test using the android API #12659

Closed
wants to merge 8 commits into from

Conversation

Craigacp
Copy link
Contributor

Description:

Adds a test which should trigger #11451 on Android. It also adds the same tests to Java, but those already passed.

I suspect the react tests could be cleaned up more, but I've no idea how they are actually run, or if there can be multiple modules, so someone who better understands how those tests are run should take a look. I don't have an environment setup to allow me to run them locally, though I have checked the new Java side tests.

cc @fs-eire

Motivation and Context

  • Why is this change required? What problem does it solve? Test for the JNI refactor when using Android & react-native.
  • If it fixes an open issue, please link to the issue here.

@fs-eire
Copy link
Contributor

fs-eire commented Aug 19, 2022

/azp run ONNX Runtime React Native CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fs-eire
Copy link
Contributor

fs-eire commented Aug 23, 2022

/azp run ONNX Runtime React Native CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fs-eire
Copy link
Contributor

fs-eire commented Aug 25, 2022

/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

@fs-eire
Copy link
Contributor

fs-eire commented Aug 25, 2022

/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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@fs-eire
Copy link
Contributor

fs-eire commented Aug 31, 2022

/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

@fs-eire
Copy link
Contributor

fs-eire commented Aug 31, 2022

/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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@Craigacp
Copy link
Contributor Author

Craigacp commented Sep 7, 2022

Looks like the training pipeline failure was due to a CI VM error, could someone re-run it?

@Craigacp
Copy link
Contributor Author

Craigacp commented Sep 15, 2022

@fs-eire please could you kick off the orttraining-linux-ci-pipeline? The failure is unrelated to this PR, it looks like the VM filled its disk up. The JNI changes have all been merged in, so this test is the last part of that work.

@fs-eire
Copy link
Contributor

fs-eire commented Sep 19, 2022

/azp run orttraining-linux-ci-pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@yuslepukhin yuslepukhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@Craigacp
Copy link
Contributor Author

Craigacp commented Nov 7, 2022

I think I made all the requested changes here. Can @yuslepukin or @fs-eire review it again?

@YUNQIUGUO
Copy link
Contributor

Is this change still applicable and should be checked-in?

@Craigacp
Copy link
Contributor Author

Yes, though it may need rebasing to make it merge cleanly. The tests always passed on Java, this adds tests which did fail on android before the JNI refactor last year and now should not.

@fs-eire
Copy link
Contributor

fs-eire commented Oct 12, 2023

/azp run ONNX Runtime React Native CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Craigacp
Copy link
Contributor Author

Looks like the react code changed a little around me, so I'll rebase this branch on main and then try to figure out what the test should look like.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@fs-eire
Copy link
Contributor

fs-eire commented Nov 7, 2023

Looks like CI was failed. could you try merge to latest main?

@fs-eire
Copy link
Contributor

fs-eire commented Nov 8, 2023

/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline

@fs-eire
Copy link
Contributor

fs-eire commented Nov 8, 2023

/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-python-checks-ci-pipeline,onnxruntime-binary-size-checks-ci-pipeline,Android CI Pipeline

@fs-eire
Copy link
Contributor

fs-eire commented Nov 8, 2023

/azp run iOS CI Pipeline,ONNX Runtime React Native CI Pipeline

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@Craigacp
Copy link
Contributor Author

Craigacp commented Nov 8, 2023

The react native stuff seems to require a new code formatter now I've rebased. It doesn't appear to be the spotless one used elsewhere in the Java code. I tried to run npm run format like it suggested but got an error message as I need to install a specific Java formatter in npm. Any idea which one it wants?

@fs-eire
Copy link
Contributor

fs-eire commented Nov 14, 2023

'npm run format' should not apply on 3rd party code which are under node_modules/ folder. Is it complaining code format for external code?

@Craigacp
Copy link
Contributor Author

It's complaining about the Java test file that I modified in this PR under js/react_native.

@fs-eire
Copy link
Contributor

fs-eire commented Nov 14, 2023

you can do either of the following as your preferred

  • check in the file after it get formatted by "npm run format"
  • or modify script line "format:cf" in js/package.json to modify the glob rule to exclude the file

@Craigacp
Copy link
Contributor Author

I had the wrong clang-format installed, it was hitting the native binary not the node wrapper around it. After installing the node clang-format it fixed a missing space and seems happy now.

@fs-eire
Copy link
Contributor

fs-eire commented Nov 15, 2023

/azp run ONNX Runtime React Native CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Craigacp Craigacp closed this Sep 9, 2024
@skottmckay
Copy link
Contributor

@Craigacp is this change out of date or would still be good to add. Happy to help getting it in if the latter. Wasn't aware of this stale PR.

@Craigacp
Copy link
Contributor Author

I think this test is still good, but I don't have an environment to run react native code so it was tricky for me to test. The Java part I'll pull out and put in another PR.

I don't think the bugs it catches are likely anymore, they went away when I completely rewrote the JNI parts to be better at exception handling. The issue with react native was that Android threw the exceptions differently to a JVM in some edge cases and so the errors that came out were weird, but with the JNI rewrite they are identical across platforms (to the best of my knowledge).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants