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

[Fix] InterOpNumThreads Session Option for ONNX ReactNative Package #21263

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

pavangoyal42
Copy link
Contributor

@pavangoyal42 pavangoyal42 commented Jul 5, 2024

Description

This PR resolves a bug related to setting the interOpNumThreads session option when creating an ORTSession. Currently, when the interOpNumThreads option is passed from React Native, the native module incorrectly sets intraOpNumThreads instead of interOpNumThreads.

Motivation and Context

Since this is a bug, users of the Onnx React Native package may believe that they are setting interOpNumThreads correctly, So this change is required. Refer to the code snippet below for details
Screenshot 2024-07-05 at 9 28 58 PM

@pavangoyal42
Copy link
Contributor Author

@snnn @skottmckay @fs-eire please review this change.

@snnn snnn added the platform:mobile issues related to ONNX Runtime mobile; typically submitted using template label Jul 8, 2024
@snnn
Copy link
Member

snnn commented Jul 8, 2024

/azp run Big Models, Linux Android Emulator QNN CI Pipeline, Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, Linux QNN CI Pipeline, MacOS CI Pipeline

@snnn
Copy link
Member

snnn commented Jul 8, 2024

/azp run Windows ARM64 QNN CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows x64 QNN CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@snnn
Copy link
Member

snnn commented Jul 8, 2024

/azp run ONNX Runtime React Native CI Pipeline

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

snnn
snnn previously approved these changes Jul 8, 2024
@pavangoyal42
Copy link
Contributor Author

@snnn is there any actionable on me for this PR to get merge ? Please check.

@snnn
Copy link
Member

snnn commented Jul 8, 2024

/azp run ONNX Runtime React Native CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavangoyal42
Copy link
Contributor Author

pavangoyal42 commented Jul 8, 2024

@snnn checked failure test case in ONNX Runtime React Native CI Pipeline (React Native CI ReactNative_CI) workflow. Below test cases are getting failed with error Test Failed: Waited for the root of the view hierarchy to have window focus and not request layout for 10 seconds. If you specified a non default root matcher, it may be picking a root that never takes focus.
Screenshot 2024-07-08 at 8 53 37 PM
This doesn't seems to related with this change. This error generally comes when a system dialog is displayed. can you please tell what went wrong here ? Thanks

@snnn
Copy link
Member

snnn commented Jul 8, 2024

@skottmckay , could you please help take a look? It fails only on this PR.

@snnn snnn dismissed their stale review July 8, 2024 17:16

Test fails. Do not merge.

@pavangoyal42
Copy link
Contributor Author

pavangoyal42 commented Jul 8, 2024

@snnn @skottmckay I have checked. below checks are failing in this PR as well #21262

  • Lint / Optional Lint (pull_request)

  • Lint / Lint C++ (pull_request)

  • ONNX Runtime React Native CI Pipeline

  • ONNX Runtime React Native CI Pipeline (React Native CI ReactNative_CI)

@snnn
Copy link
Member

snnn commented Jul 10, 2024

/azp run ONNX Runtime React Native CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavangoyal42
Copy link
Contributor Author

pavangoyal42 commented Jul 10, 2024

@snnn Thanks for giving another run for react native checks. After rerun these checks are passed. I can see checks related to lint and c++ lint are still failing. Regarding the c++ lint fixes PR is already raised #21303, for Lint/ Optional lint check looks like there is typo in function description comment. Please refer attached screen shot.
Screenshot 2024-07-10 at 11 44 49 AM
src="https://github.com/microsoft/onnxruntime/assets/88655321/f2a392d7-4c78-48a8-85fa-f0726090d01d">
Let me know if these 2 checks are blocker for this PR to merge and how can I help you fix these. Thanks!!

Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

Thank you!

@snnn snnn merged commit 1b82d83 into microsoft:main Jul 10, 2024
68 of 70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:mobile issues related to ONNX Runtime mobile; typically submitted using template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants