Make grammar creation deterministic

[aphedges]

I was encountering a bug in our agent where the same input DAIDE was being parsed properly by daidepp in some cases but not in others. I assumed it was the fault of the agent, but I ended up creating a minimal reproduction using only daidep:

from daidepp import PRP, create_daide_grammar, daide_visitor

ALL_GRAMMAR = create_daide_grammar(level=160, string_type="all", test=True)
msg = "PRP(XDO((ENG AMY LVP) MTO WAL))"
parse_tree = ALL_GRAMMAR.parse(msg)
parsed_msg = daide_visitor.visit(parse_tree)
print(f"parsed_msg = {parsed_msg!r}")
print(f"type(parsed_msg) = {type(parsed_msg)}")
assert isinstance(parsed_msg, PRP)

The string PRP(XDO((ENG AMY LVP) MTO WAL)) should always be parsed to an object of type PRP, but it was often being parsed to a list instead. Running the above script repeatedly will show the output can differ.

I ended up figuring out that daidepp was generating grammars non-deterministically across runs. Sometimes the grammar ended up leading to a different, incorrect parse, and I was able to even show that (with a low probability) parsing can fail entirely. I have modified this library to generate the grammar deterministically. I also fixed a related bug that prevented proper parsing of arrangements and made some smaller code changes to improve some problems I found during my work.

Notes

To repeatedly run the above Python script, I used the following Bash script:

#!/usr/bin/env bash

for ((i = 0; i < 10; i++)); do
  python daide_test.py
done

To view the grammar being generated, I used the following patch:

diff --git a/src/daidepp/grammar/grammar_utils.py b/src/daidepp/grammar/grammar_utils.py
index d2b8c5e..cc69d97 100644
--- a/src/daidepp/grammar/grammar_utils.py
+++ b/src/daidepp/grammar/grammar_utils.py
@@ -45,6 +45,7 @@ def create_daide_grammar(
     level: Union[DAIDELevel, List[DAIDELevel]] = 30,
     allow_just_arrangement: bool = False,
     string_type: Literal["message", "arrangement", "all"] = "message",
+    test: bool = False,
 ) -> DAIDEGrammar:
     """Create a DAIDEGrammar object given a level of DAIDE.

@@ -71,6 +72,10 @@ def create_daide_grammar(
         string_type = "arrangement"

     grammar_str = _create_daide_grammar_str(level, string_type)
+    if test:
+        from pathlib import Path
+        from time import time_ns
+        Path("grammars", f"{string_type}_{level}_{time_ns()}.txt").write_text(grammar_str)
     grammar = DAIDEGrammar(grammar_str)
     return grammar

It assumes that a directory named grammars already exists. This specific diff used 6c10f70 as its base.

[aphedges]

Thanks for merging!

By the way, I did intend for this to merged without squashing and wrote the commit history carefully for that. For future PRs (it's obviously too late for this one now), should I note that if it's what I want? I had expected it here because previous PRs weren't squashed. I'm just not really sure about the conventions for this repo.

[mjspeck]

Thanks for merging!

By the way, I did intend for this to merged without squashing and wrote the commit history carefully for that. For future PRs (it's obviously too late for this one now), should I note that if it's what I want? I had expected it here because previous PRs weren't squashed. I'm just not really sure about the conventions for this repo.

Sorry about that! Lemme double check about whether that would conflict with our auto-versioning system. If it's fine I could revert the merge and we could re-merge without the squash if you'd like. I'll also update the PR template for people to specify whether they want to squash or not.

[aphedges]

Sorry about that! Lemme double check about whether that would conflict with our auto-versioning system. If it's fine I could revert the merge and we could re-merge without the squash if you'd like.

It's not a big deal. I don't think it's worth the effort to do it now.

Thanks for the offer, though!