Feature/mmsf validator #301
Conversation
| @@ -58,7 +59,7 @@ def cache_component(max_channels=2): | |||
| def echo_component(max_channels=2): | |||
| ports = {Operator.F_INIT: [f'in{i+1}' for i in range(max_channels)], | |||
| Operator.O_F: [f'out{i+1}' for i in range(max_channels)]} | |||
| instance = Instance(ports, KEEPS_NO_STATE_FOR_NEXT_USE) | |||
| instance = Instance(ports, KEEPS_NO_STATE_FOR_NEXT_USE | SKIP_MMSF_SEQUENCE_CHECKS) | |||
There was a problem hiding this comment.
Hmm. Maybe it would be nicer to modify this so that it receives all messages into a list, and then sends them all out? Then it would be MMSF-compatible, and we could run with checks enabled. Of course it's a test, so it's fine also because we know what we're doing.
There was a problem hiding this comment.
I left it as is: I think it's also nice to have an integration test for this flag.
| 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): |
There was a problem hiding this comment.
self._current_operator.allows_receiving() would be better I think, because it doesn't duplicate the information on which ports can send or receive.
I think Operator.NONE allows both actually, there should probably be a branch for that too.
There was a problem hiding this comment.
Or maybe Operator.NONE should be deprecated, now that I think of it. I think it came from MUSCLE2, the idea being to allow any kind of generic message exchange, but it seems like everything people want to do fits the MMSF, even if it's generic RPC. And we can disable the checks if needed. Let's add a branch and leave it like that for now.
There was a problem hiding this comment.
In this logic I'm using Operator.NONE to signal a reuse-instance. I'm assuming there are no ports with this operator.
Do you think it's logical to add a check in MMSFValidator.__init__ that there are no ports with Operator.NONE?
There was a problem hiding this comment.
If you would? Then people could still use it, but they'd have to disable the validator and that definitely makes sense.
There was a problem hiding this comment.
Agreed. Added a warning in MMSFValidator.__init__ (python) and constructor (C++)
… port allows sending/receiving
Also fix a python test and comment
And implement checks in send() and receive() to check if the port's operator allows sending/receiving.
| 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 | ||
| 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
| continue; // op is not in the allowed list | ||
| for (auto const & to_op : allowed_transitions_[op]) { | ||
| // add to_op to allowed, if it is not already in the list: | ||
| if (std::find(allowed.begin(), allowed.end(), to_op) == allowed.end()) |
There was a problem hiding this comment.
Didn't you write a helper function for this std::find?
| check_transition(op, port_name); | ||
| } | ||
|
|
||
| if (std::find( |
There was a problem hiding this comment.
Maybe the helper helps here too?
There was a problem hiding this comment.
Yes I did, after I had to repeat this too often. Apparently I wasn't sharp enough to replace all instances of std::find I'd written up to that point 😅
|
|
||
| private: | ||
| /** Actual implementation of check_send/check_receive. */ | ||
| void check_send_receive(std::string const & port_name, Optional<int> slot); |
There was a problem hiding this comment.
Can you add a trailing underscore to the name of these private members? It's not written down anywhere, but it is the standard used in MUSCLE3.
| 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): |
There was a problem hiding this comment.
If you would? Then people could still use it, but they'd have to disable the validator and that definitely makes sense.
And small changes based on PR feedback
Add a validator that checks whether messages are sent and received in the order prescribed by the MMSF.
TODO