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

[NOT FOR MERGING] iOS Task Library Prototype #3893

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

priankakariatyml
Copy link
Contributor

  1. Prototyped the iOS task library by developing the text classifier APIs.
  2. Only includes text classifier init methods.

@priankakariatyml
Copy link
Contributor Author

@schmidt-sebastian @khanhlvg Raised PR for iOS task library prototype

* The base class of the user-facing iOS mediapipe text task api classes.
*/
NS_SWIFT_NAME(BaseTextTaskApi)
@interface MPPBaseTextTaskApi : NSObject {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schmidt-sebastian @khanhlvg
This is the base class for all text APIs. Please note that C++ header files are included here. These headers and any headers transitively imported by them will have to be part of the iOS framework we build for distribution. According to me this is not a good design as consumers of the API will be exposed to many C++ headers. Also I am yet to check the feasibility of this design when building CocoaPods.

My Suggestion:
Instead of extending the individual tasks from a base task api, we can create a base task util class which will encapsulate the c++ task runner and all the common functionality needed. The utils will only be included in the .mm file. So we don't have to expose the C++ headers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate on this? The headers will be part of the build process, but does the user have to consume them in order to use our iOS API?

NS_SWIFT_NAME(BaseTextTaskApi)
@interface MPPBaseTextTaskApi : NSObject {
@protected
std::unique_ptr<mediapipe::tasks::core::TaskRunner> cppTaskRunner;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schmidt-sebastian @khanhlvg @lu-wang-g
I am using the C++ task runner like the Python implementation. Java on the other hand, creates its own task runner with the graph APIs.
If need be, we can follow the Java approach. Only thing is, we will have to rewrite MPPGraph and add a lot of other classes to process packets etc. The current iOS graph APIs depend on MPPGraph. If we use this approach, we will have to ensure, we don't break anything.

LMK if sticking to the Python approach is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will be much better off in the long run if we follow the Python approach and re-use as much code as possible.

}

- (MPPClassificationResult *)classifyWithText:(NSString *)text error:(NSError **)error {
Packet packet = [MPPPacketCreator createWithText:text];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need MPPPacketCreator ? Can't we just do MakePacketstd::string(text.cppString) here?

Copy link
Collaborator

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Approving this so I can test importing. This is not yet ready to be merged.

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.

None yet

3 participants