Skip to content

Conversation

@YUNQIUGUO
Copy link
Contributor

@YUNQIUGUO YUNQIUGUO commented Apr 22, 2023

Description

js/react_native package dependency change to manage ort-extensions for react-native app.

Enable optional inclusion of ort-ext aar/ ort-ext pods for react-native extensions apps when specifiy ortExtensionsEnabled in user's package.json file

Motivation and Context

@YUNQIUGUO YUNQIUGUO marked this pull request as ready for review April 26, 2023 08:47
#import <onnxruntime/onnxruntime_cxx_api.h>

#ifdef ENABLE_ORT_EXT
#include <onnxruntime_extensions.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

so this include doesn't require any leading directory?

Copy link
Contributor Author

@YUNQIUGUO YUNQIUGUO Apr 26, 2023

Choose a reason for hiding this comment

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

correct, this one works fine. it's the <onnxruntime_c_api.h> this extensions header includes requires a leading directory.

Copy link
Contributor Author

@YUNQIUGUO YUNQIUGUO Apr 27, 2023

Choose a reason for hiding this comment

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

update: actually looks like just getting rid of https://github.com/microsoft/onnxruntime-extensions/blob/0e2d6c03e4b87f49e6d35c1ca540e99317ac8bb9/includes/onnxruntime_extensions.h#L5
this line here would work with the project. (i.e. no "redefinition" error at compile stage for objects defined in onnxruntime_c_api.h)

I guess this c_api include in extensions header isn't really necessary.

Copy link
Contributor

@skottmckay skottmckay Apr 28, 2023

Choose a reason for hiding this comment

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

Does this require a change on the ort extensions side for it to work? If so are there any objections to that change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wenbingl what do you think? could we remove the onnxruntime_c_api.h include from here and just forward declare the ORT types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this require a change on the ort extensions side for it to work? If so are there any objections to that change?

yeah... hopefully we can just remove that line as Ed pined above.

Copy link
Contributor

@wenbingl wenbingl Apr 28, 2023

Choose a reason for hiding this comment

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

@wenbingl what do you think? could we remove the onnxruntime_c_api.h include from here and just forward declare the ORT types?

onnxruntime_extensions.h is the only ABI head file for ort-extensions, and Register... function depends on some types defined c_api.h. Can we assume that every time the onnxruntime_extensions.h is included, onnxruntme_c_api.h already included before that?

I don't know what the issue is here? redefinition? Suppose there will be only one copy of onnxruntime_c_api.h in the whole build since ort-extensions can use onnxruntime_c_api.h from internal folder, if not, that's something should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what the issue is here? redefinition? Suppose there will be only one copy of onnxruntime_c_api.h in the whole build since ort-extensions can use onnxruntime_c_api.h from internal folder, if not, that's something should be fixed.

yea, I experienced a redefinition error when building a react native app on the ios side. I guess it duplicates with the onnxruntime_c_api.h header that already been included from the onnxruntime-c pod.

I will try to seek ways to not making changes on the ort-extensions side first.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ort header file are setting here: https://github.com/microsoft/onnxruntime-
extensions/blob/f12270b06858b2a57c6fa5041c39d835a707060f/cmake/ext_ortlib.cmake#L2
You may make some change here if needed to make sure the right header file can be found.

import ai.onnxruntime.OrtSession.SessionOptions;
import ai.onnxruntime.extensions.OrtxPackage;

class OnnxruntimeExtensions {
Copy link
Contributor Author

@YUNQIUGUO YUNQIUGUO Apr 27, 2023

Choose a reason for hiding this comment

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

not specifying public here for this class as other wise compiler would complain about the class name not match the file name. but in our case, it should be fine as if not specified, will by default be package-specific. We only will call and use this class within the same package bundle ai.onnxruntime.reactnative


class OnnxruntimeExtensions {
public void registerOrtExtensionsIfEnabled(SessionOptions sessionOptions) {
Log.d("Info", "ORT Extensions is not enabled.");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we update the message to tell them how to enable it as clearly they're wanting to?

public void registerOrtExtensionsIfEnabled(SessionOptions sessionOptions) {
Log.d(
"Info",
"ORT Extensions is not enabled in the current configuration. If you want to enable this support,"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add space

Suggested change
"ORT Extensions is not enabled in the current configuration. If you want to enable this support,"
"ORT Extensions is not enabled in the current configuration. If you want to enable this support, "

Copy link
Contributor

Choose a reason for hiding this comment

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

also the first argument is a tag, maybe it should be something like "OnnxruntimeExtensions". if you want an info log instead of debug, use Log.i()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right got it. thanks!

edgchen1
edgchen1 previously approved these changes Apr 29, 2023
@YUNQIUGUO YUNQIUGUO merged commit c8bd34f into main Apr 29, 2023
@YUNQIUGUO YUNQIUGUO deleted the yguo/rn-ext-package branch April 29, 2023 07:07
ShukantPal pushed a commit to ShukantPal/onnxruntime that referenced this pull request May 7, 2023
…native app (microsoft#15641)

### Description
<!-- Describe your changes. -->

js/react_native package dependency change to manage ort-extensions for
react-native app.

Enable optional inclusion of ort-ext aar/ ort-ext pods for react-native
extensions apps when specifiy `ortExtensionsEnabled` in user's
package.json file


### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->

---------

Co-authored-by: rachguo <rachguo@rachguos-Mac-mini.local>
Co-authored-by: rachguo <rachguo@rachguos-Mini.attlocal.net>
siweic0 pushed a commit to siweic0/onnxruntime-web that referenced this pull request May 9, 2024
…native app (microsoft#15641)

### Description
<!-- Describe your changes. -->

js/react_native package dependency change to manage ort-extensions for
react-native app.

Enable optional inclusion of ort-ext aar/ ort-ext pods for react-native
extensions apps when specifiy `ortExtensionsEnabled` in user's
package.json file


### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->

---------

Co-authored-by: rachguo <rachguo@rachguos-Mac-mini.local>
Co-authored-by: rachguo <rachguo@rachguos-Mini.attlocal.net>
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