diff --git a/pyproject.toml b/pyproject.toml index 2798f5b..8a56650 100644 --- a/pyproject.toml +++ b/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" }] diff --git a/src/reynir/eparser.cpp b/src/reynir/eparser.cpp index 83d0d62..fcc9bdf 100644 --- a/src/reynir/eparser.cpp +++ b/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 { diff --git a/src/reynir/eparser.h b/src/reynir/eparser.h index a9b5ae8..bc74f52 100644 --- a/src/reynir/eparser.h +++ b/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*); diff --git a/src/reynir/grammar.py b/src/reynir/grammar.py index 083fa25..f0e6bfe 100644 --- a/src/reynir/grammar.py +++ b/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 diff --git a/test/test_native_matching.py b/test/test_native_matching.py index 04911df..1e0ac81 100644 --- a/test/test_native_matching.py +++ b/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: diff --git a/test/test_parse.py b/test/test_parse.py index 0fa1a4e..707d33c 100644 --- a/test/test_parse.py +++ b/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 diff --git a/uv.lock b/uv.lock index d1fdaad..9b33966 100644 --- a/uv.lock +++ b/uv.lock @@ -409,7 +409,7 @@ wheels = [ [[package]] name = "reynir" -version = "3.7.1" +version = "3.7.2" source = { editable = "." } dependencies = [ { name = "cffi" },