Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

[PIO-193] Async support for predict method and storage access, blocking code wrapped in blocking construct #495

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

longliveenduro
Copy link
Contributor

@longliveenduro longliveenduro commented Nov 6, 2018

I tried to leverage the already present async interface of elasticsearch and armouring the HBase and JDBC call with blocking constructs which will tell the standard scala ExecutionContext to use a separate thread for that.

Let me know what you think.

See https://issues.apache.org/jira/browse/PIO-193
and
https://lists.apache.org/thread.html/f14e4f8f29410e4585b3d8e9f646b88293a605f4716d3c4d60771854@%3Cuser.predictionio.apache.org%3E
and also https://jira.apache.org/jira/browse/PIO-182
for more details

See also actionml/universal-recommender#62 for corresponding UR PR

@takezoe
Copy link
Member

takezoe commented Nov 7, 2018

Thanks for great work. However I think this way isn't acceptable because it breaks current interfaces. This means that all existing templates won't work after this change.

Async support in entire prediction service is really nice but we need to find another way.

@longliveenduro
Copy link
Contributor Author

@takezoe yes, you are definitely right. I'm just thinking about and trying some backward compatible tweaks. Will get back!

@dszeto
Copy link
Contributor

dszeto commented Nov 7, 2018

Thank you @longliveenduro ! Fingers crossed for your update. :)

@longliveenduro
Copy link
Contributor Author

longliveenduro commented Nov 9, 2018

@dszeto @takezoe This would be one possible solution to be nearly 100% backward compatible. With "nearly" I mean that an engine implementor has to add "override" to its implementation of predictBase() / predict(), when he recompiles/changes his engine. Because in this suggestion I implement it with a default NotImplementedError stating that the method is deprecated. But this should only be necessary when the engine developer recompiles his engine. With that "override" hint he then also gets a good hint to move on to the new async implementation. Already compiled engines should still work as the simply override the new default impl. of predict/predictBase. predictBaseAsync and predictAsync simply delegate to the old methods and wrapping a blocking block around it to tell the standard scala execution context to execute them in a second, much bigger threadpool. See chapter "Blocking" from https://www.beyondthelines.net/computing/scala-future-and-execution-context/

If you want we could just drop this default implementation of BaseAlgorithm.predictBase / Subclass.predict and would have 100% compatibility but then even a new async implementation would have to implement these old method signatures with a dummy.

Other solution I did think of were using some runtime information (things like checking dynamically if a method is present or not) but I decided not to use that, because these kind of lookups are usually very slow and IMHO breaks type saftey.

longliveenduro pushed a commit to longliveenduro/universal-recommender that referenced this pull request Nov 9, 2018
@takezoe
Copy link
Member

takezoe commented Nov 17, 2018

I haven't checked this pull request in details, but first of all, I would like to discuss about our direction about this topic.

Basically, I agree to introduce asynchronousity in whole predict API process and I think your pull request is one of good ideas to achive it. Since we already have Akka-HTTP and async storage access layer, making prediction service asynchronous makes sense. In addition, if we can do it, we can also mark LEventStore's blocking methods as deprecated.

With "nearly" I mean that an engine implementor has to add "override" to its implementation of predictBase() / predict(), when he recompiles/changes his engine.

However, I'm not sure if this is acceptable in our backword compatibility policy. And also we need to update docs and all existing official templates to use new async APIs if we accept this pull request. It's a big change.

@dszeto What do you think about this?

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

Successfully merging this pull request may close these issues.

3 participants