Multi-model support #692

cowchipkid · 2018-09-05T14:28:14Z

This version eliminates the reliance on static singletons for configuration parameters, models, brown clusters and gazetteers for NER.

…tatic singleton rather they are stored in the annotator (which is a singleton per model via the factory for NER annotators). In this way, we can have multiple NER models for different languages in the same process space.

cowchipkid · 2018-09-05T14:31:08Z

@mssammon @danyaljj I noticed when I ran the script to increment the version number, there were a slew of .gitignore files that were modified as well. I have not noticed this before, did the script do this? Should I commit these changes as well?

danyaljj · 2018-09-05T14:46:04Z

@cowchipkid not that I know of. Unless it's necessary, please don't.

mssammon

Overall, looks really good.
I've marked a couple of catch blocks that simply print an error, but don't otherwise interfere with program execution. I think failure to load a resource should result in an exception: if the client wants to ignore it, then they should do so consciously. It is generally undesirable to run without realizing some resource is not available.

mssammon · 2018-09-05T14:33:46Z

md/src/main/java/org/cogcomp/md/BIOCombinedReader.java

@@ -153,15 +156,13 @@ else if (mode.contains("ALL")){
            bcsl.add(false);
            bcsl.add(false);
            bcsl.add(false);
-            BrownClusters.init(bcs, bcst, bcsl, false);
+            brownClusters = BrownClusters.get(bcs, bcst, bcsl);
            WordNetManager.loadConfigAsClasspathResource(true);
            wordNet = WordNetManager.getInstance();
        }
        catch (Exception e){
            e.printStackTrace();


please throw an exception here unless there is a recovery option based on a reported failure.

mssammon · 2018-09-05T14:34:26Z

md/src/main/java/org/cogcomp/md/BIOReader.java

@@ -155,15 +158,14 @@ private void annotateTas(){
            bcsl.add(false);
            bcsl.add(false);
            bcsl.add(false);
-            BrownClusters.init(bcs, bcst, bcsl, false);
+            brownClusters = BrownClusters.get(bcs, bcst, bcsl);
            WordNetManager.loadConfigAsClasspathResource(true);
            wordNet = WordNetManager.getInstance();
        }
        catch (Exception e){
            e.printStackTrace();


throw exception

I agree completely, however, this is not my code, it's nothing that I have changed, it has been this way. I can throw the exceptions up, but the exception apparently end up being thrown in the reader generation code inside the LBJava stuff, and again, I don't even know what this code does. So I can make the change, I can throw the exception up to the reader, but I can't change the getReader method signature of the LBJ generated code, or at least I shouldn't. I propose we throw the exceptions, but in the constructor for this reader, catch the thrown exceptions, convert them to a runtime exception and throw that so we don't have to change the get reader method. And the author should be made aware that this sort of absence of thought about error handling is wrong minded and unacceptable.

I realize this is not your coding practice, just saw it in the context of your changes. I think your proposal is good. If you think it will take too much time right now, you can skip making the change, but open an issue that describes them.

I have made the change, and from just a cursor glance, I think it will work out just fine.

mssammon · 2018-09-05T14:36:42Z

md/src/main/java/org/cogcomp/md/ExtentReader.java

@@ -135,14 +138,11 @@ public String getId(){
            bcsl.add(false);
            bcsl.add(false);
            bcsl.add(false);
-            BrownClusters.init(bcs, bcst, bcsl, false);
-
+            brownClusters = BrownClusters.get(bcs, bcst, bcsl);
        }
        catch (Exception e){
            e.printStackTrace();


throw exception

mssammon · 2018-09-05T14:37:01Z

md/src/main/java/org/cogcomp/md/ExtentTester.java

@@ -227,13 +230,11 @@ public static void testExtentOnGoldHead(){
            bcsl.add(false);
            bcsl.add(false);
            bcsl.add(false);
-            BrownClusters.init(bcs, bcst, bcsl, false);
+            brownClusters = BrownClusters.get(bcs, bcst, bcsl);
        }
        catch (Exception e){
            e.printStackTrace();


throw exception

mssammon · 2018-09-05T14:37:21Z

md/src/main/java/org/cogcomp/md/ExtentTester.java

@@ -306,13 +309,11 @@ public static void testExtentOnPredictedHead(){
            bcsl.add(false);
            bcsl.add(false);
            bcsl.add(false);
-            BrownClusters.init(bcs, bcst, bcsl, false);
+            brownClusters = BrownClusters.get(bcs, bcst, bcsl);
        }
        catch (Exception e){
            e.printStackTrace();


throw exception

mssammon · 2018-09-05T14:38:06Z

md/src/main/java/org/cogcomp/md/MentionAnnotator.java

@@ -181,8 +181,7 @@ else if (_mode.contains("ERE")){
            bcsl.add(false);
            bcsl.add(false);
            bcsl.add(false);
-            BrownClusters.init(bcs, bcst, bcsl, false);
-            brownClusters = BrownClusters.get();
+            brownClusters = BrownClusters.get(bcs, bcst, bcsl);
            WordNetManager.loadConfigAsClasspathResource(true);
            wordNet = WordNetManager.getInstance();
        }


throw exception in following catch block

mssammon · 2018-09-05T15:53:36Z

ner/src/main/java/edu/illinois/cs/cogcomp/ner/LbjTagger/Data.java

@@ -25,7 +25,6 @@
    HashSet<String> labelsToAnonymizeForEvaluation = new HashSet<>();

    private Data(Data other) {


I think it's OK to leave (or update) the original comment to make it immediately clear to any other dev why this is specified.

mssammon · 2018-09-05T15:56:27Z

ner/src/main/java/edu/illinois/cs/cogcomp/ner/LbjTagger/NETagPlain.java


        // NOTICE: this only checks tagger1 because tagger2 may legally be null.
-        if (tagger1 == null) {
+        if (params.taggerLevel1 == null) {
            logger.error("Tagger1 is null. You may need to call NETagPlain.init() first.");


is it correct to return null here rather than throwing an exception?

Not sure, but as the above, it has always been this way. I personally don't use this method any more.

OK. Fine to leave as is, then.

danyaljj

Besides Mark's comments, looks good to me.

mssammon

LGTM. Thanks for adding the exception handling.

Thomas L. Redman added 2 commits September 1, 2018 19:00

Next version.

1e896a6

cowchipkid requested review from mssammon and danyaljj September 5, 2018 14:28

mssammon requested changes Sep 5, 2018

View reviewed changes

danyaljj reviewed Sep 5, 2018

View reviewed changes

Added a modicum of error handling to bits of code around what I touched.

653e4ff

mssammon approved these changes Sep 5, 2018

View reviewed changes

mssammon merged commit 7d9dad3 into CogComp:master Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-model support #692

Multi-model support #692

cowchipkid commented Sep 5, 2018

cowchipkid commented Sep 5, 2018

danyaljj commented Sep 5, 2018

mssammon left a comment

mssammon Sep 5, 2018

mssammon Sep 5, 2018

cowchipkid Sep 5, 2018

mssammon Sep 5, 2018

cowchipkid Sep 5, 2018

mssammon Sep 5, 2018

mssammon Sep 5, 2018

mssammon Sep 5, 2018

mssammon Sep 5, 2018

mssammon Sep 5, 2018

mssammon Sep 5, 2018

cowchipkid Sep 5, 2018

mssammon Sep 5, 2018

danyaljj left a comment

mssammon left a comment

		@@ -25,7 +25,6 @@
		HashSet<String> labelsToAnonymizeForEvaluation = new HashSet<>();

		private Data(Data other) {

Multi-model support #692

Multi-model support #692

Conversation

cowchipkid commented Sep 5, 2018

cowchipkid commented Sep 5, 2018

danyaljj commented Sep 5, 2018

mssammon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danyaljj left a comment

Choose a reason for hiding this comment

mssammon left a comment

Choose a reason for hiding this comment