From 51d37c34a45bd738e90649041f5a47dbca9ac80e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 15:10:21 -0700 Subject: [PATCH 1/7] Reimplement checking of ports arity --- src/qref/verification.py | 88 ++++++++++++++----- .../qref/data/invalid_topology_programs.yaml | 20 ++++- tests/qref/test_topology_verification.py | 7 +- 3 files changed, 86 insertions(+), 29 deletions(-) diff --git a/src/qref/verification.py b/src/qref/verification.py index 2b0fac7..d7d2634 100644 --- a/src/qref/verification.py +++ b/src/qref/verification.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from collections import defaultdict +from collections import Counter, defaultdict from dataclasses import dataclass from .functools import accepts_all_qref_types @@ -50,13 +50,17 @@ def verify_topology(routine: SchemaV1 | RoutineV1) -> TopologyVerificationOutput return TopologyVerificationOutput(problems) -def _verify_routine_topology(routine: RoutineV1) -> list[str]: +def _verify_routine_topology(routine: RoutineV1, ancestor_path: tuple[str] = ()) -> list[str]: adjacency_list = _get_adjacency_list_from_routine(routine, path=None) return [ - *_find_cycles(adjacency_list), - *_find_disconnected_ports(routine), - *[problem for child in routine.children for problem in _verify_routine_topology(child)], + *_find_cycles(adjacency_list, ancestor_path), + *_find_disconnected_ports(routine, ancestor_path), + *[ + problem + for child in routine.children + for problem in _verify_routine_topology(child, ancestor_path + (routine.name,)) + ], ] @@ -99,16 +103,16 @@ def _get_adjacency_list_from_routine(routine: RoutineV1, path: str | None) -> Ad return graph -def _find_cycles(adjacency_list: AdjacencyList) -> list[str]: +def _find_cycles(adjacency_list: AdjacencyList, ancestor_path: tuple[str]) -> list[str]: # Note: it only returns the first detected cycle. for node in list(adjacency_list): - problem = _dfs_iteration(adjacency_list, node) + problem = _dfs_iteration(adjacency_list, node, ancestor_path) if problem: return problem return [] -def _dfs_iteration(adjacency_list: AdjacencyList, start_node: str) -> list[str]: +def _dfs_iteration(adjacency_list: AdjacencyList, start_node: str, ancestor_path: tuple[str]) -> list[str]: to_visit: list[str] = [start_node] visited: list[str] = [] predecessors: dict[str, str] = {} @@ -122,29 +126,67 @@ def _dfs_iteration(adjacency_list: AdjacencyList, start_node: str) -> list[str]: # Reconstruct the cycle cycle = [neighbour] while len(cycle) < 2 or cycle[-1] != start_node: - cycle.append(predecessors[cycle[-1]]) + cycle.append(".".join(ancestor_path + (predecessors[cycle[-1]],))) return [f"Cycle detected: {cycle[::-1]}"] if neighbour not in visited: to_visit.append(neighbour) return [] -def _find_disconnected_ports(routine: RoutineV1) -> list[str]: +def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str]) -> list[str]: problems: list[str] = [] + + def _prefix(name: str) -> str: + return ".".join(ancestor_path + (routine.name, name)) + + sources_counts = Counter[str]() + target_counts = Counter[str]() + + for connection in routine.connections: + sources_counts[connection.source] += 1 + target_counts[connection.target] += 1 + + multi_sources = [source for source, count in sources_counts.items() if count > 1] + + multi_targets = [target for target, count in target_counts.items() if count > 1] + + if multi_sources: + problems.append(f"Too many outgoing connections from {','.join(_prefix(target) for target in multi_sources)}.") + + if multi_targets: + problems.append(f"Too many incoming connections to {','.join(_prefix(target) for target in multi_targets)}.") + + requiring_outgoing = set[str]() + requiring_incoming = set[str]() + cannot_be_connected = set[str]() + + for port in routine.ports: + if port.direction == "input" and routine.children: + requiring_outgoing.add(port.name) + elif port.direction == "output" and routine.children: + requiring_incoming.add(port.name) + elif port.direction == "through": # Note: through ports have to be valid regardless of existence of children + cannot_be_connected.add(port.name) + for child in routine.children: + # Directions are reversed compared to parent + through ports have to be connected on both ends for port in child.ports: - pname = f"{routine.name}.{child.name}.{port.name}" - if port.direction == "input": - matches_in = [c for c in routine.connections if c.target == f"{child.name}.{port.name}"] - if len(matches_in) == 0: - problems.append(f"No incoming connections to {pname}.") - elif len(matches_in) > 1: - problems.append(f"Too many incoming connections to {pname}.") - elif port.direction == "output": - matches_out = [c for c in routine.connections if c.source == f"{child.name}.{port.name}"] - if len(matches_out) == 0: - problems.append(f"No outgoing connections from {pname}.") - elif len(matches_out) > 1: - problems.append(f"Too many outgoing connections from {pname}.") + pname = f"{child.name}.{port.name}" + if port.direction != "output": + requiring_incoming.add(pname) + if port.direction != "input": + requiring_outgoing.add(pname) + + for port in requiring_outgoing: + if port not in sources_counts: + problems.append(f"No outgoing connection from {_prefix(port)}.") + + for port in requiring_incoming: + if port not in target_counts: + problems.append(f"No incoming connection to {_prefix(port)}.") + + for port in cannot_be_connected: + if port in sources_counts or port in target_counts: + problems.append(f"A through port {_prefix(port)} is connected via an internal connection.") return problems diff --git a/tests/qref/data/invalid_topology_programs.yaml b/tests/qref/data/invalid_topology_programs.yaml index d9b2620..0e9b181 100644 --- a/tests/qref/data/invalid_topology_programs.yaml +++ b/tests/qref/data/invalid_topology_programs.yaml @@ -128,7 +128,23 @@ version: v1 description: Program has badly connected ports problems: - - "No incoming connections to root.child_1.in_1." + - "No incoming connection to root.child_1.in_1." - "Too many incoming connections to root.child_2.in_0." - "Too many outgoing connections from root.child_2.out_0." - - "No outgoing connections from root.child_2.out_1." + - "No outgoing connection from root.child_2.out_1." +- input: + version: v1 + program: + name: root + ports: + - direction: through + name: thru_0 + size: 1 + - direction: output + name: out_0 + size: 1 + connections: + - thru_0 -> out_0 + description: "A routine with its thru port connected to its output port." + problems: + - "A through port root.thru_0 is connected via an internal connection." diff --git a/tests/qref/test_topology_verification.py b/tests/qref/test_topology_verification.py index e38e37a..7c0c95f 100644 --- a/tests/qref/test_topology_verification.py +++ b/tests/qref/test_topology_verification.py @@ -45,7 +45,6 @@ def test_correct_routines_pass_topology_validation(valid_program): def test_invalid_program_fails_to_validate_with_schema_v1(input, problems): verification_output = verify_topology(SchemaV1(**input)) - assert not verification_output - assert len(problems) == len(verification_output.problems) - for expected_problem, problem in zip(problems, verification_output.problems): - assert expected_problem == problem + # We use sorted here, to make sure that we don't test the order in which the + # problems appear, as the order is only an implementation detail. + assert sorted(verification_output.problems) == sorted(problems) From 47844b4b477faeb75c510334363562307e987149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 15:49:01 -0700 Subject: [PATCH 2/7] Streamline handling of paths --- src/qref/verification.py | 42 ++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/qref/verification.py b/src/qref/verification.py index d7d2634..284ae40 100644 --- a/src/qref/verification.py +++ b/src/qref/verification.py @@ -14,6 +14,7 @@ from collections import Counter, defaultdict from dataclasses import dataclass +from typing import Callable from .functools import accepts_all_qref_types from .schema_v1 import RoutineV1, SchemaV1 @@ -36,7 +37,7 @@ def __bool__(self) -> bool: @accepts_all_qref_types -def verify_topology(routine: SchemaV1 | RoutineV1) -> TopologyVerificationOutput: +def verify_topology(routine: RoutineV1) -> TopologyVerificationOutput: """Checks whether program has correct topology. Correct topology cannot include cycles or disconnected ports. @@ -44,14 +45,19 @@ def verify_topology(routine: SchemaV1 | RoutineV1) -> TopologyVerificationOutput Args: routine: Routine or program to be verified. """ - if isinstance(routine, SchemaV1): - routine = routine.program problems = _verify_routine_topology(routine) return TopologyVerificationOutput(problems) -def _verify_routine_topology(routine: RoutineV1, ancestor_path: tuple[str] = ()) -> list[str]: - adjacency_list = _get_adjacency_list_from_routine(routine, path=None) +def _make_prefixer(ancestor_path: tuple[str, ...]) -> Callable[[str], str]: + def _prefix(text: str) -> str: + return ".".join([*ancestor_path, text]) + + return _prefix + + +def _verify_routine_topology(routine: RoutineV1, ancestor_path: tuple[str, ...] = ()) -> list[str]: + adjacency_list = _get_adjacency_list_from_routine(routine, path=ancestor_path) return [ *_find_cycles(adjacency_list, ancestor_path), @@ -64,7 +70,7 @@ def _verify_routine_topology(routine: RoutineV1, ancestor_path: tuple[str] = ()) ] -def _get_adjacency_list_from_routine(routine: RoutineV1, path: str | None) -> AdjacencyList: +def _get_adjacency_list_from_routine(routine: RoutineV1, path: tuple[str, ...]) -> AdjacencyList: """This function creates a flat graph representing one hierarchy level of a routine. Nodes represent ports and edges represent connections (they're directed). @@ -72,15 +78,12 @@ def _get_adjacency_list_from_routine(routine: RoutineV1, path: str | None) -> Ad into the children, and from the children into all the output ports. """ graph = defaultdict[str, list[str]](list) - if path is None: - current_path = routine.name - else: - current_path = ".".join([path, routine.name]) + _prefix = _make_prefixer(path + (routine.name,)) # First, we go through all the connections and add them as adges to the graph for connection in routine.connections: - source = ".".join([current_path, connection.source]) - target = ".".join([current_path, connection.target]) + source = _prefix(connection.source) + target = _prefix(connection.target) graph[source].append(target) # Then for each children we add an extra node and set of connections @@ -88,7 +91,7 @@ def _get_adjacency_list_from_routine(routine: RoutineV1, path: str | None) -> Ad input_ports: list[str] = [] output_ports: list[str] = [] - child_path = ".".join([current_path, child.name]) + child_path = _prefix(child.name) for port in child.ports: if port.direction == "input": input_ports.append(".".join([child_path, port.name])) @@ -103,7 +106,7 @@ def _get_adjacency_list_from_routine(routine: RoutineV1, path: str | None) -> Ad return graph -def _find_cycles(adjacency_list: AdjacencyList, ancestor_path: tuple[str]) -> list[str]: +def _find_cycles(adjacency_list: AdjacencyList, ancestor_path: tuple[str, ...]) -> list[str]: # Note: it only returns the first detected cycle. for node in list(adjacency_list): problem = _dfs_iteration(adjacency_list, node, ancestor_path) @@ -112,11 +115,13 @@ def _find_cycles(adjacency_list: AdjacencyList, ancestor_path: tuple[str]) -> li return [] -def _dfs_iteration(adjacency_list: AdjacencyList, start_node: str, ancestor_path: tuple[str]) -> list[str]: +def _dfs_iteration(adjacency_list: AdjacencyList, start_node: str, ancestor_path: tuple[str, ...]) -> list[str]: to_visit: list[str] = [start_node] visited: list[str] = [] predecessors: dict[str, str] = {} + _prefix = _make_prefixer(ancestor_path) + while to_visit: node = to_visit.pop() visited.append(node) @@ -126,18 +131,17 @@ def _dfs_iteration(adjacency_list: AdjacencyList, start_node: str, ancestor_path # Reconstruct the cycle cycle = [neighbour] while len(cycle) < 2 or cycle[-1] != start_node: - cycle.append(".".join(ancestor_path + (predecessors[cycle[-1]],))) + cycle.append(_prefix(predecessors[cycle[-1]])) return [f"Cycle detected: {cycle[::-1]}"] if neighbour not in visited: to_visit.append(neighbour) return [] -def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str]) -> list[str]: +def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str, ...]) -> list[str]: problems: list[str] = [] - def _prefix(name: str) -> str: - return ".".join(ancestor_path + (routine.name, name)) + _prefix = _make_prefixer(ancestor_path + (routine.name,)) sources_counts = Counter[str]() target_counts = Counter[str]() From 9dd7d52bbd84749e5a5bd7e7117cc224a58c5590 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 15:50:20 -0700 Subject: [PATCH 3/7] Improve naming --- src/qref/verification.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qref/verification.py b/src/qref/verification.py index 284ae40..076195c 100644 --- a/src/qref/verification.py +++ b/src/qref/verification.py @@ -162,7 +162,7 @@ def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str, ...]) requiring_outgoing = set[str]() requiring_incoming = set[str]() - cannot_be_connected = set[str]() + thru_ports = set[str]() for port in routine.ports: if port.direction == "input" and routine.children: @@ -170,7 +170,7 @@ def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str, ...]) elif port.direction == "output" and routine.children: requiring_incoming.add(port.name) elif port.direction == "through": # Note: through ports have to be valid regardless of existence of children - cannot_be_connected.add(port.name) + thru_ports.add(port.name) for child in routine.children: # Directions are reversed compared to parent + through ports have to be connected on both ends @@ -189,7 +189,7 @@ def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str, ...]) if port not in target_counts: problems.append(f"No incoming connection to {_prefix(port)}.") - for port in cannot_be_connected: + for port in thru_ports: if port in sources_counts or port in target_counts: problems.append(f"A through port {_prefix(port)} is connected via an internal connection.") From 3c351cbf1b2fc6a23d17dc2e93718a98f7aca8c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 16:00:19 -0700 Subject: [PATCH 4/7] Add new example that would trip previous implementation --- .../qref/data/invalid_topology_programs.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/qref/data/invalid_topology_programs.yaml b/tests/qref/data/invalid_topology_programs.yaml index 0e9b181..3dd45ba 100644 --- a/tests/qref/data/invalid_topology_programs.yaml +++ b/tests/qref/data/invalid_topology_programs.yaml @@ -148,3 +148,45 @@ description: "A routine with its thru port connected to its output port." problems: - "A through port root.thru_0 is connected via an internal connection." +- input: + version: v1 + program: + name: root + ports: + - direction: input + name: in_0 + size: N + - direction: output + name: out_0 + size: N + - direction: output + name: out_1 + size: N + connections: + - in_0 -> foo.in_0 + - foo.out_0 -> out_0 + - foo.out_1 -> out_1 + children: + - name: foo + ports: + - direction: input + name: in_0 + size: N + - direction: output + name: out_0 + size: N + - direction: output + name: out_1 + size: N + connections: + - bar.out_0 -> out_1 + children: + - name: bar + ports: + - name: out_0 + size: N + direction: output + description: "Program with disconnected container ports" + problems: + - "No outgoing connection from root.foo.in_0." + - "No incoming connection to root.foo.out_0." From 877c80c030f3f2afc36d09b0676db034e80ef69d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 16:16:41 -0700 Subject: [PATCH 5/7] Fix malformed f-String --- src/qref/schema_v1.py | 2 +- tests/qref/test_schema_validation.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/qref/schema_v1.py b/src/qref/schema_v1.py index 766877b..b096f29 100644 --- a/src/qref/schema_v1.py +++ b/src/qref/schema_v1.py @@ -208,7 +208,7 @@ def _validate_connections(self) -> Self: if missed_ports: raise ValueError( "The following ports appear in a connection but are not " - "among routine's port or their children's ports: {missed_ports}." + f"among routine's port or their children's ports: {missed_ports}." ) return self diff --git a/tests/qref/test_schema_validation.py b/tests/qref/test_schema_validation.py index 1e37538..32dd322 100644 --- a/tests/qref/test_schema_validation.py +++ b/tests/qref/test_schema_validation.py @@ -44,3 +44,21 @@ def test_invalid_program_fails_to_validate_with_pydantic_model_v1(input): def test_valid_program_succesfully_validate_with_pydantic_model_v1(valid_program): SchemaV1.model_validate(valid_program) + + +def test_validation_error_includes_name_of_the_missed_port(): + input = { + "version": "v1", + "program": { + "name": "root", + "ports": [{"name": "in_0", "direction": "input", "size": 1}], + "connections": ["in_0 -> out_0"], + }, + } + + pattern = ( + "The following ports appear in a connection but are not among routine's port " + r"or their children's ports: \['out_0'\]." # <- out_0 is the important bit here + ) + with pytest.raises(pydantic.ValidationError, match=pattern): + SchemaV1.model_validate(input) From 011f9e7a690e298282b83cc5852ed23408e0cf42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 16:21:16 -0700 Subject: [PATCH 6/7] Tidy up imports --- src/qref/verification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qref/verification.py b/src/qref/verification.py index 076195c..d56a334 100644 --- a/src/qref/verification.py +++ b/src/qref/verification.py @@ -17,7 +17,7 @@ from typing import Callable from .functools import accepts_all_qref_types -from .schema_v1 import RoutineV1, SchemaV1 +from .schema_v1 import RoutineV1 AdjacencyList = dict[str, list[str]] From 1050f7707bdff800771d1a541cd4e53bad4ebe87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Ja=C5=82owiecki?= Date: Mon, 30 Sep 2024 16:25:59 -0700 Subject: [PATCH 7/7] Fix name clash --- src/qref/verification.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qref/verification.py b/src/qref/verification.py index d56a334..4222936 100644 --- a/src/qref/verification.py +++ b/src/qref/verification.py @@ -181,16 +181,16 @@ def _find_disconnected_ports(routine: RoutineV1, ancestor_path: tuple[str, ...]) if port.direction != "input": requiring_outgoing.add(pname) - for port in requiring_outgoing: - if port not in sources_counts: - problems.append(f"No outgoing connection from {_prefix(port)}.") + for pname in requiring_outgoing: + if pname not in sources_counts: + problems.append(f"No outgoing connection from {_prefix(pname)}.") - for port in requiring_incoming: - if port not in target_counts: - problems.append(f"No incoming connection to {_prefix(port)}.") + for pname in requiring_incoming: + if pname not in target_counts: + problems.append(f"No incoming connection to {_prefix(pname)}.") - for port in thru_ports: - if port in sources_counts or port in target_counts: - problems.append(f"A through port {_prefix(port)} is connected via an internal connection.") + for pname in thru_ports: + if pname in sources_counts or pname in target_counts: + problems.append(f"A through port {_prefix(pname)} is connected via an internal connection.") return problems