Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/mmsf validator #301

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
60 changes: 28 additions & 32 deletions libmuscle/python/libmuscle/mmsf_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,36 +52,37 @@ def __init__(self, port_manager: PortManager) -> None:

# Allowed operator transitions, the following are unconditionally allowed:
self._allowed_transitions = {
(Operator.NONE, Operator.NONE),
(Operator.NONE, Operator.F_INIT),
(Operator.F_INIT, Operator.O_I),
(Operator.F_INIT, Operator.O_F),
(Operator.O_I, Operator.S),
(Operator.S, Operator.O_I),
(Operator.S, Operator.O_F),
(Operator.O_F, Operator.NONE),
}
Operator.NONE: [Operator.NONE, Operator.F_INIT],
Operator.F_INIT: [Operator.O_I, Operator.O_F],
Operator.O_I: [Operator.S],
Operator.S: [Operator.O_I, Operator.O_F],
Operator.O_F: [Operator.NONE]}
# If there are operators without connected ports, we can skip over those
maarten-ic marked this conversation as resolved.
Show resolved Hide resolved
for operator in [Operator.F_INIT, Operator.O_I, Operator.S, Operator.O_F]:
if not self._connected_ports.get(operator, []):
# Find all transitions A -> operator -> B and allow transition A -> B:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realise this before but this is actually a bit more involved, because we're editing _allowed_transitions on the fly. So this will also enable e.g. NONE -> S if F_INIT and O_I have no ports. That's what we want, but not immediately obvious. Maybe this comment could be extended to point out that it's transitive to avoid future confusion?

Also, in that case, will we end up with None -> O_I in _allowed_transitions, despite O_I not having connected ports? That's probably not a problem, since that transition cannot occur anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this comment could be extended to point out that it's transitive to avoid future confusion?

Sure, will do

Also, in that case, will we end up with None -> O_I in _allowed_transitions, despite O_I not having connected ports? That's probably not a problem, since that transition cannot occur anyway?

Correct, and it is deliberately so. Users can still send on an O_I port when it is not connected (the Communicator will just discard the message). For the MMSFValidator this transition still happens and is valid :)

skip_from = []
skip_to = []
for from_op, to_op in self._allowed_transitions:
for from_op, allowed in self._allowed_transitions.items():
if from_op is operator:
skip_to.append(to_op)
if to_op is operator:
skip_from.append(from_op)
for from_op in skip_from:
for to_op in skip_to:
self._allowed_transitions.add((from_op, to_op))
continue
if operator not in allowed:
continue
for to_op in self._allowed_transitions[operator]:
if to_op not in allowed:
allowed.append(to_op)
# Sort allowed transitions for more logical log messages
for allowed in self._allowed_transitions.values():
allowed.sort(key=lambda op: op.value)

# Disable this validator when the instance uses vector ports to keep this class
# simpler. Support for vector ports may be added in the future.
self._enabled = not any(
port.is_vector() for ports in port_objects.values() for port in ports)
_logger.debug(
"MMSF Validator is %s", "enabled" if self._enabled else "disabled")
if self._enabled:
_logger.debug("MMSF Validator is enabled")
else:
_logger.debug(
"MMSF Validator is disabled: this instance uses vector ports, "
"which are not supported by the MMSF Validator.")

# State tracking
self._current_ports_used: List[str] = []
Expand Down Expand Up @@ -143,30 +144,25 @@ def _check_transition(self, operator: Operator, port_name: str = "") -> None:
if port not in self._current_ports_used]
if unused_ports:
# We didn't complete the current phase
if self._current_operator in (Operator.F_INIT, Operator.S):
if self._current_operator.allows_receiving():
expected = "a receive"
else:
expected = "a send"
expected += (
f" on any of these {self._current_operator.name} ports: "
+ ", ".join(unused_ports))

elif (self._current_operator, operator) not in self._allowed_transitions:
# Transition to the operator is not allowed, now figure out what we were
# actually expecting.
# First find the allowed transitions from self._current_operator, that are
# also 'valid' (i.e. have connected ports):
allowed = [
to_op for from_op, to_op in self._allowed_transitions
if from_op is self._current_operator and
(to_op in self._connected_ports or to_op is Operator.NONE)]
elif operator not in self._allowed_transitions[self._current_operator]:
# Transition to the operator is not allowed.
# Build the message we want to display to users:
expected_lst = []
for to_op in sorted(allowed, key=lambda op: op.value):
for to_op in self._allowed_transitions[self._current_operator]:
ports = ', '.join(map(repr, self._connected_ports.get(to_op, [])))
if to_op is Operator.NONE:
LourensVeen marked this conversation as resolved.
Show resolved Hide resolved
expected_lst.append("a call to reuse_instance()")
elif to_op in (Operator.F_INIT, Operator.S):
elif not ports:
continue
elif to_op.allows_receiving():
expected_lst.append(f"a receive on an {to_op.name} port ({ports})")
else:
expected_lst.append(f"a send on an {to_op.name} port ({ports})")
Expand Down