-
Notifications
You must be signed in to change notification settings - Fork 449
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
OPENNLP-936: Add thread-safe versions of POSTaggerME, SentenceDetecto… #69
Conversation
Thanks!
|
Ok done. |
opennlp-tools/src/test/java/opennlp/tools/eval/MultiThreadedToolsEval.java
Show resolved
Hide resolved
*/ | ||
public class POSTaggerME_TS implements POSTagger { | ||
|
||
private POSModel model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POSModel is not thread-safe because GISModel uses HashMap which is not thread-safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to change the values in the map after the model is create. It should be a Collections.unmodifiableMap()... which would be Thread.Safe().
This issue (OPENNLP-936) is still relevant to many users. Is there any form of consensus - in 2022 - on how to progress with the current code base? |
The comments on this PR seem fairly trivial. I have no objections to getting those comments addressed and merging this. I will go through the PR again and see if there's anything since then that should be addressed. Thanks @mawiesne for bringing attention to this one. |
opennlp-tools/src/main/java/opennlp/tools/postag/POSTaggerME_TS.java
Outdated
Show resolved
Hide resolved
Hi all, looks like this thread has broadly reached consensus back in 2022, but not yet been merged - what needs to happen to get it looked at again? I've verified that the current version of the SentenceDetectorME indeed suffer from this issue and implementing the solution originally proposed by @twgoetz fixes it. |
@bachelarius Think it boils down to (a) rename the classes, (b) add the @threadsafe annotation for tracking and (c) rebase against latest changes from main. |
That being said: PRs are welcome. |
Thread safe versions of POSTaggerME, SentenceDetectorME and TokenizerME. Include test case as well.
I rebased this PR, so we get CI feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eval build no. 26 passed: https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/26/ |
…rME and TokenizerME. Include test case as well.
I'm open to changing the names of the classes, if anybody has a better idea.