fix(ontology): accept verbalizations in any role order - #456
tripleaceme wants to merge 2 commits into
Conversation
_parse_verbalization() compared each {Concept} token with the role at the
same position, so a reverse reading such as '{Company} employs {Person}'
for a relationship declared Person -> Company was rejected.
Tokens are now matched to roles by concept name and role name. A reading
that uses a role twice is rejected. When a concept plays several unnamed
roles, its tokens take them in declared order, as before. The declared
roles are unchanged; RelationshipVerbalization.roles follows the reading.
Closes apache#445
kayemkim
left a comment
There was a problem hiding this comment.
Thanks for this, and for checking the consumers of the parsed roles and the #412 interaction before anyone asked. Both hold up: on main nothing outside model.py and the tests reads .verbalizations, the round-trips write verbalizes_raw, and #412 does iterate verbalizations[0].roles at its line 537.
Ran the branch merged into current main (938a439) through the ontology CI steps on Python 3.11 to 3.14: green, 148 tests against 143 on main. The issue's YAML fails on main with the Role 0 error and parses here. I also had a real document to try: an ontology of mine written in July, 18 relationships with Korean concept names and 19 readings, one of them written from the other side ({고객}이 {주문}을 했다 under the 주문 concept). Main rejects it with the same Role 0 error, this branch parses it, and the non-ASCII names go through the token regex fine. Ten hand-written edge cases (reverse ternary, ring with and without role names, wrong case, role name on an unnamed role, a dropped role, wrong arity, whitespace inside tokens) give the same verdict on both trees except the reorderings this PR is meant to accept. #412, #389, #441 and #454 all merge with this without conflict. LGTM from me.
One small suggestion on the error text. For a ring with a named role, parenthood with roles Person and Person:parent, the reading {Person} has parent {Person} now fails with "uses role '{Person}' more than once". The actual mistake is the missing :parent on the second token, and the old message at least pointed at Person:parent. When every match is already used but an unused role of the same concept exists under a different name, naming that role in the message would send the author to the right fix. Not blocking.
For a ring with a named role, a reading such as '{Person} has parent
{Person}' failed with "uses role '{Person}' more than once", which does
not say what to change. When the concept still has an unused role under
another name, the error now names it.
|
Thanks @kayemkim, and for trying it on a real document with non-ASCII names. Good catch on the ring error. I pushed a change: when a token matches only roles that are already used, and the same concept still has an unused role under another name, the error now names it. For A plain repeated role with no such alternative keeps the original message. Added a test for this case; the ontology suite passes (149). |
jbonofre
left a comment
There was a problem hiding this comment.
Thanks @tripleaceme for addressing this and adding solid test coverage, and thanks @kayemkim for the thorough review and testing with real-world ontologies.
LGTM!
Summary
_parse_verbalization()compared each{Concept}token with the role at the same position, so a relationship could only be verbalized in its declared role order. Forworks forwith roles Person, Company, the reading{Company} employs {Person}raised:Tokens are now matched to roles by concept name, and by role name when one is given:
{Person} manages {Person}), its tokens take those roles in declared order, as before.{Person:parent} is parent of {Person}works.RelationshipVerbalization.rolesnow follows the order of the reading, and the docstring says so.On the question in the issue about whether anything relies on role order: nothing on
maindoes. The only consumers of the parsed roles are inmodel.pyand the tests. The spec, YAML and OWL round-trips write the rawverbalizesstrings, so their output is unchanged.One thing for reviewers to know: open PR #412 builds its RelationalAI reading from
verbalizations[0].rolesin list order. With this change, if a user writes a reverse reading first, that PR's field order would follow the reading. #412 also movestests/test_verbalization_parsing.py, so whichever merges second will need to move these tests.Related Issues
Closes #445
Tests
New tests in
tests/test_verbalization_parsing.py:The two existing wrong-concept and wrong-role tests were updated for the new error message.
cd converters/ontology && uv run pytest: 148 passed