Fix many flaky tests due to non-determinism of hash map #1218
Conversation
|
I understand that conceptually there's no guarantee a java map iterates items in the same order they were inserted, but I have never seen this cause errors in the tests in practice. That's on multiple architectures and JVMs. I'd be very curious to know what setup you are using which is causing this to happen. At any rate, without extensively testing the difference in memory & speed performance, I'm not up for switching HashMap -> LinkedHashMap. I'd be happy to integrate changes to the tests themselves which eliminate the flakiness, though. |
|
Yes you are correct that you nor I may be able to reproduce this error on demand. However, since it can occur at any point in time (running on a particular JVM), I try to fix these flaky tests before it comes a bigger issue. I use a tool call NONDEX to detect such tests; but I have not yet tried running the code on different JVMs to reproduce the error. With regards to the 10 tests which assumes the iterator returns the elements in an order, I don't think it's possible to change the tests alone to make them non-flaky. More specifically, although I can, in principle, simply check that the list contains a particular string (rather than assuming the string exists at a particular index), it will not be a sufficient test of the underlying methods. The underlying methods (or code) relies on a specific order (which isn't guaranteed with a HashMap); hence my suggestion to use a LinkedHashMap. The other two test methods ( |
|
Upon further investigation, both I understand that you're not prepared to make that adjustment but I've made these suggestions since this issue can show its 'ugly head' at any point in time or upon a change in JVM — which has happened many times on other projects. |
|
I'm totally fine with making a smaller slice of the codebase use a LinkedHashMap. I just don't want Generics.newHashMap() to change without understanding the memory and speed implications of that. Semgrex expressions themselves are far more expensive than the map itself, so there's no reason not to make that change in that module. (Compare this with the conparser's use of maps for features, for example, where the map is on the critical path and is the main use of memory.) Also, I think I understand now why this issue has never come up before. It appears that having two attributes at once on a node is not part of the unit test. Oops! Thanks for your persistence - the extra bit of information here really helped narrow down the issue. |
|
Oh, I see and I fully understand your concern. Yes, I should have made the change to the submodule instead, as to prevent any unintended consequences. Thanks for pointing that out. Glad everything is solved now. |
|
Great. I merged my version of the PR. It also had an expanded unit test,
which did in fact show the error you're talking about here. (Now fixed,
obviously!) I merged it to the dev branch. Would you like to check if
there are other remaining non-deterministic tests?
|
|
Okay, so I took a closer look. Your fix solved the last two problems: My new fix fixes all but
|
README.md
Outdated
| @@ -40,7 +40,7 @@ At present, [the current released version of the code](https://stanfordnlp.githu | |||
| 1. Make sure you have Maven installed, details here: [https://maven.apache.org/](https://maven.apache.org/) | |||
| 2. If you run this command in the CoreNLP directory: `mvn package` , it should run the tests and build this jar file: `CoreNLP/target/stanford-corenlp-4.3.2.jar` | |||
| 3. When using the latest version of the code make sure to download the latest versions of the [corenlp-models](http://nlp.stanford.edu/software/stanford-corenlp-models-current.jar), [english-extra-models](http://nlp.stanford.edu/software/stanford-english-extra-corenlp-models-current.jar), and [english-kbp-models](http://nlp.stanford.edu/software/stanford-english-kbp-corenlp-models-current.jar) and include them in your CLASSPATH. If you are processing languages other than English, make sure to download the latest version of the models jar for the language you are interested in. | |||
| 4. If you want to use Stanford CoreNLP as part of a Maven project you need to install the models jars into your Maven repository. Below is a sample command for installing the Spanish models jar. For other languages just change the language name in the command. To install `stanford-corenlp-models-current.jar` you will need to set `-Dclassifier=models`. Here is the sample command for Spanish: `mvn install:install-file -Dfile=/location/of/stanford-spanish-corenlp-models-current.jar -DgroupId=edu.stanford.nlp -DartifactId=stanford-corenlp -Dversion=4.2.2 -Dclassifier=models-spanish -Dpackaging=jar` | |||
| 4. If you want to use Stanford CoreNLP as part of a Maven project you need to install the models jars into your Maven repository. Below is a sample command for installing the Spanish models jar. For other languages just change the language name in the command. To install `stanford-corenlp-models-current.jar` you will need to set `-Dclassifier=models`. Here is the sample command for Spanish: `mvn install:install-file -Dfile=/location/of/stanford-spanish-corenlp-models-current.jar -DgroupId=edu.stanford.nlp -DartifactId=stanford-corenlp -Dversion=4.3.2 -Dclassifier=models-spanish -Dpackaging=jar` | |||
There was a problem hiding this comment.
@J38 has already done this in both main & dev
There was a problem hiding this comment.
I confirm this was a bug in GitHub. I switched the upstream branch to "main" and then back to "dev", and now the correct git diff is shown.
|
In thinking about the semgrex and SemanticGraph maps, I think the
SemanticGraph map is far more relevant to the actual use of the tools.
The SemanticGraph maps are used for iterating to follow the edges from a
node, for example, and the order there can change the order results are
returned in. I think both the "inner" and "outer" map would benefit from
turning into LinkedHashMap, as then nodes and edges would both be iterated
in an expected order.
namesToNodes and namesToRelations is iterated in outputResults in the
SemgrexTest, but only for visualizing the results. Is that what the test
checking tool is finding? Regardless, I don't see much harm in changing
the types used, and it would make the results more visually consistent at
least.
…On Thu, Dec 2, 2021 at 2:32 PM Auzel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#1218 (comment)>:
> @@ -40,7 +40,7 @@ At present, [the current released version of the code](https://stanfordnlp.githu
1. Make sure you have Maven installed, details here: [https://maven.apache.org/](https://maven.apache.org/)
2. If you run this command in the CoreNLP directory: `mvn package` , it should run the tests and build this jar file: `CoreNLP/target/stanford-corenlp-4.3.2.jar`
3. When using the latest version of the code make sure to download the latest versions of the [corenlp-models](http://nlp.stanford.edu/software/stanford-corenlp-models-current.jar), [english-extra-models](http://nlp.stanford.edu/software/stanford-english-extra-corenlp-models-current.jar), and [english-kbp-models](http://nlp.stanford.edu/software/stanford-english-kbp-corenlp-models-current.jar) and include them in your CLASSPATH. If you are processing languages other than English, make sure to download the latest version of the models jar for the language you are interested in.
-4. If you want to use Stanford CoreNLP as part of a Maven project you need to install the models jars into your Maven repository. Below is a sample command for installing the Spanish models jar. For other languages just change the language name in the command. To install `stanford-corenlp-models-current.jar` you will need to set `-Dclassifier=models`. Here is the sample command for Spanish: `mvn install:install-file -Dfile=/location/of/stanford-spanish-corenlp-models-current.jar -DgroupId=edu.stanford.nlp -DartifactId=stanford-corenlp -Dversion=4.2.2 -Dclassifier=models-spanish -Dpackaging=jar`
+4. If you want to use Stanford CoreNLP as part of a Maven project you need to install the models jars into your Maven repository. Below is a sample command for installing the Spanish models jar. For other languages just change the language name in the command. To install `stanford-corenlp-models-current.jar` you will need to set `-Dclassifier=models`. Here is the sample command for Spanish: `mvn install:install-file -Dfile=/location/of/stanford-spanish-corenlp-models-current.jar -DgroupId=edu.stanford.nlp -DartifactId=stanford-corenlp -Dversion=4.3.2 -Dclassifier=models-spanish -Dpackaging=jar`
I confirm this was a bug in GitHub. I switched the upstream branch to
"main" and then back to "dev", and now the correct git diff is shown.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1218 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2AYWLVWYHOVJ7W5UEBYEDUO7XZJANCNFSM5HXHJFTQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
As for the test that changed, it was simply a reflection of the ordering of some outputs being changed. Not a big deal |
|
Okay, noted. I took a look at the changes you've made and realized it hasn't been merged as yet. I'll rerun the tests after the merge to ensure all the (implementation-dependent) flaky tests have been eliminated. |
|
Sounds good. It is merged now. I left you as the author for the change to
LinkedHashMap. Thanks for the help!
|
|
Okay, nice! I have since check the dev branch and all flaky tests have been eliminated. So, when the dev branch is merged into main, everything will be good. So I'll close the PR now. |
|
Thanks again!
…On Wed, Dec 8, 2021, 9:34 AM Auzel ***@***.***> wrote:
Okay, nice! I have since check the dev branch and all flaky tests have
been eliminated. So, when the dev branch is merged into main, everything
will be good. So I'll close the PR now.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1218 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2AYWMAYHI2NBA7NGIB5KTUP6JJTANCNFSM5HXHJFTQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Following up on our earlier conversation, I have found the following flaky tests in this project:
edu.stanford.nlp.semgraph.semgrex.SemgrexTest.testNegatedAttribute edu.stanford.nlp.semgraph.semgrex.SemgrexTest.testComplicatedGraph edu.stanford.nlp.semgraph.semgrex.SemgrexTest.testNotEquals edu.stanford.nlp.semgraph.semgrex.ProcessSemgrexRequestTest.testTwoGraphs edu.stanford.nlp.semgraph.semgrex.ProcessSemgrexRequestTest.testSimpleRequest edu.stanford.nlp.semgraph.semgrex.ProcessSemgrexRequestTest.testSingleMultiRequest edu.stanford.nlp.semgraph.semgrex.ProcessSemgrexRequestTest.testTwoSemgrex edu.stanford.nlp.trees.UniversalEnglishGrammaticalStructureTest.doTest edu.stanford.nlp.semgraph.semgrex.SemgrexTest.testEqualsRelation edu.stanford.nlp.semgraph.semgrex.ProcessSemgrexRequestTest.testDoubleMultiRequest edu.stanford.nlp.semgraph.semgrex.SemgrexTest.testAttributeConjunction edu.stanford.nlp.naturalli.RelationTripleSegmenterTest.testAmericanActorChrisPratt edu.stanford.nlp.semgraph.semgrex.ProcessSemgrexRequestTest.testUnclosedMultiRequestAll but
edu.stanford.nlp.semgraph.semgrex.SemgrexTest.testNegatedAttributeandedu.stanford.nlp.semgraph.semgrex.SemgrexTest.testAttributeConjunctionassume that the iterator of theHashMapreturns the values in the order they were stored. However, this is not true in java since values are returned in a non-deterministic order.My fix is to let the underlying data structure be a
LinkedHashMap,as opposed to aHashMap. This will ensure that the values are returned in the expected order. The performance differences of the two data structures are negligible but the change toLinkedHashMapwill eliminate 10 flaky tests.