Merge pull request #66 from mideind/fix/deterministic-reduction

Make parse results deterministic across runs

Author: vthorsteinsson

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "reynir"
-version = "3.7.1"
+version = "3.7.2"
 description = "A natural language parser for Icelandic"
 authors = [{ name = "Miðeind ehf.", email = "mideind@mideind.is" }]
 maintainers = [{ name = "Miðeind ehf.", email = "mideind@mideind.is" }]

src/reynir/eparser.cpp

@@ -115,10 +115,18 @@ friend class AllocReporter;
 
    UINT getHash(void) const
       {
-         return ((UINT)this->m_iNt) ^
-            ((UINT)((uintptr_t)this->m_pProd) & 0xFFFFFFFF) ^
-            (this->m_nDot << 7) ^ (this->m_nStart << 9) ^
-            (((UINT)((uintptr_t)this->m_pw) & 0xFFFFFFFF) << 1);
+         // Content-based hash: this must not depend on memory
+         // addresses, since the hash determines the order in which
+         // states are enumerated from a Column's hash bins, which
+         // in turn determines the order of families of children
+         // in the resulting parse forest. Using pointer values here
+         // would make parse results nondeterministic between runs
+         // whenever the reducer encounters exact score ties.
+         UINT h = ((UINT)this->m_iNt) ^
+            (this->m_nDot << 7) ^ (this->m_nStart << 9);
+         h = h * 31 + (this->m_pProd ? this->m_pProd->getId() : (UINT)-1);
+         h = h * 31 + (this->m_pw ? this->m_pw->getHash() : 0);
+         return h;
       }
    BOOL operator==(const State& other) const
       {

src/reynir/eparser.h

@@ -270,6 +270,20 @@ class Label {
             this->m_nJ == other.m_nJ;
       }
 
+   // Content-based hash, deliberately independent of memory
+   // addresses (the production is represented by its id, not
+   // its pointer), so that hash-derived orderings are stable
+   // between runs
+   UINT getHash(void) const
+      {
+         UINT h = (UINT)this->m_iNt;
+         h = h * 31 + this->m_nDot;
+         h = h * 31 + (this->m_pProd ? this->m_pProd->getId() : (UINT)-1);
+         h = h * 31 + this->m_nI;
+         h = h * 31 + this->m_nJ;
+         return h;
+      }
+
 };
 
 
@@ -310,6 +324,10 @@ class Node {
    BOOL hasLabel(const Label& label) const
       { return this->m_label == label; }
 
+   // Deterministic content-based hash of this node's label
+   UINT getHash(void) const
+      { return this->m_label.getHash(); }
+
    void dump(Grammar*);
 
    static UINT numCombinations(Node*);

src/reynir/grammar.py

@@ -178,12 +178,15 @@ def __init__(self, name: str, fname: Optional[str] = None, line: int = 0) -> Non
         # Give all nonterminals a unique, negative sequence number for hashing purposes
         self._index = Nonterminal._index
         Nonterminal._index -= 1
-        self._hash = id(self).__hash__()
+        # Use the creation-order sequence number as the hash. It is
+        # deterministic between runs (unlike id()), so that iteration
+        # order over sets and dicts of nonterminals is stable, and it
+        # never changes during the object's lifetime (unlike self._index,
+        # which may be renumbered after the grammar has been processed).
+        self._hash = self._index
 
     def __hash__(self) -> int:
-        """Use the id of this nonterminal as a basis for the hash"""
-        # The index may change after the entire grammar has been
-        # read and processed; therefore it is not suitable for hashing
+        """Return the cached, deterministic hash of this nonterminal"""
         return self._hash
 
     def __eq__(self, o: Any) -> bool:
@@ -274,8 +277,13 @@ def __init__(self, name: str) -> None:
         self._name = name
         self._index = Terminal._index
         Terminal._index += 1
-        # The hash is used quite often so it is worth caching
-        self._hash = id(self).__hash__()
+        # The hash is used quite often so it is worth caching.
+        # Use the creation-order sequence number: it is deterministic
+        # between runs (unlike id()), so that iteration order over sets
+        # and dicts of terminals is stable, and it never changes during
+        # the object's lifetime (unlike self._index, which may be
+        # renumbered after the grammar has been processed).
+        self._hash = self._index
 
     def __hash__(self) -> int:
         return self._hash

test/test_native_matching.py

@@ -131,10 +131,11 @@
 def _parse_results(disable_native: bool):
     """Parse the corpus with a fresh parser, native matching on or off.
     Returns, per sentence, the number of parse tree combinations in the
-    forest (or None if the sentence did not parse). Note that we compare
-    forest sizes rather than reduced trees, since the reducer may break
-    exact score ties differently between runs; the forest itself is
-    fully determined by the token/terminal match results."""
+    forest (or None if the sentence did not parse). We compare forest
+    sizes since they are fully determined by the token/terminal match
+    results, which is exactly what this module tests. (Reduced trees
+    are nowadays deterministic as well; see test_parse.py::
+    test_deterministic_reduction.)"""
     key = "GREYNIR_DISABLE_CPP_MATCHING"
     old = os.environ.get(key)
     try:

test/test_parse.py

@@ -2209,6 +2209,29 @@ def test_þau(r):
     # assert s.tree.S.IP.NP_SUBJ.PP.NP.tidy_text == "þeim Gunnlaugi"
 
 
+def test_deterministic_reduction(r):
+    """Repeated parses of the same sentence must yield the same tree,
+    even when the parse forest contains subtrees with exactly equal
+    scores. The ambiguous sentences below used to come out differently
+    between runs when the C++ parser's Earley state hash included
+    memory addresses, making the family order in the forest - and
+    thereby the reducer's tie-breaking - nondeterministic."""
+    sentences = [
+        "Ása sá sól.",
+        "Konan sem kom í heimsókn í gær ætlar að kaupa nýja íbúð í miðbænum.",
+        "Hr. Jón Jónsson býr á Laugavegi 26 og á 3,4 milljónir króna í banka.",
+        "Það rignir sjaldan í Reykjavík í júlí en þó gerist það stundum.",
+        "Tuttugu og þrír hestar, fimm kýr og tólf kindur voru á bænum.",
+    ]
+    for sent in sentences:
+        flats = set()
+        for _ in range(5):
+            s = r.parse_single(sent)
+            assert s is not None and s.tree is not None
+            flats.add(s.tree.flat)
+        assert len(flats) == 1, f"Nondeterministic parse of '{sent}': {flats}"
+
+
 def test_aukafall(r):
     s = r.parse_single("Mér blöskrar framkoma Páls.")
     assert s and s.tree