Message interface is hidden inside function call #301
Conversation
unkcpz
force-pushed
the
message-wrapping-pause-kill-same-api
branch
2 times, most recently
from
62f8b88 to
560098c
Compare
tests/test_processes.py
Outdated
| self.assertTrue(proc.killed()) | ||
| self.assertEqual(proc.killed_msg(), msg) | ||
| self.assertEqual(proc.killed_msg()[MESSAGE_KEY], msg_text) |
There was a problem hiding this comment.
I don't understand this change 🤔 MESSAGE_KEY is imported and passed as key, why is that?
There was a problem hiding this comment.
It is basically the same as previous test. The previous one build the msg which is a dict and I use it to assert. Now, the msg is not build, so I directly compare the msg_text. The different here is with new change, the proc.kill only require passing the msg_text, rather than construct the message type first.
There was a problem hiding this comment.
As we discussed in the office, let's make it this way:
| self.assertEqual(proc.killed_msg()[MESSAGE_KEY], msg_text) | |
| self.assertEqual(proc.killed_msg()[MessageBuilder.MESSAGE_TEXT_KEY], msg_text) |
There was a problem hiding this comment.
@khsrali also prefer having directly using proc.killed_msg()['message'] which I don't agree with both.
I agree with changing the name to MESSAGE_TEXT_KEY can be a bit more clear. But I don't like putting this as a class attribute.
@sphuber what you think? It is mostly a matter of taste and we need the third one to settle it.
from plumpy.process_comms import MESSAGE_TEXT_KEY
msg = proc.killed_msg()
msg_text = msg[MESSAGE_TEXT_KEY]from plumpy.process_comms import MessageBuilder
msg = proc.killed_msg()
msg_text = msg[MessageBuilder.MESSAGE_TEXT_KEY]msg = proc.killed_msg()
msg_text = msg['message']There was a problem hiding this comment.
The point is, why should these keys be outside of the only class that defines them? they are defined as MessageBuilder keys.
from plumpy.process_comms import MESSAGE_TEXT_KEY, FORCE_KILL_KEY, INTENT_KEY
msg = proc.killed_msg()
msg_text = msg[MESSAGE_TEXT_KEY]
intent = msg[INTENT_KEY]
kill_flag = msg[FORCE_KILL_KEY]whilst you could:
from plumpy.process_comms import MessageBuilder as mb
msg = proc.killed_msg()
msg_text = msg[mb.MESSAGE_TEXT_KEY]
intent = msg[mb.INTENT_KEY]
kill_flag = msg[mb.FORCE_KILL_KEY]or even:
msg = proc.killed_msg()
msg_text = msg['MESSAGE_TEXT']
intent = msg['INTENT']
kill_flag = msg['FORCE_KILL']
src/plumpy/processes.py
Outdated
| if state_msg is None: | ||
| msg_text = '' | ||
| else: | ||
| msg_text = state_msg[MESSAGE_KEY] |
There was a problem hiding this comment.
these lines are new, why is that
There was a problem hiding this comment.
Because _do_pause in _create_interrupt_action is taking a message rather than a msg_text as input. So either I change it there or here accept msg_text. Since _do_pause is private method, so I just didn't bother with passing msg_text as other public API.
I think for the internal communication it can use messagetype, and only when for public APIs, in this PR I let the function as the contract for what types need to be as inputs.
src/plumpy/process_comms.py
Outdated
| @@ -401,28 +401,24 @@ def play_all(self) -> None: | |||
| """ | |||
| self._communicator.broadcast_send(None, subject=Intent.PLAY) | |||
|
|
|||
| def kill_process(self, pid: 'PID_TYPE', msg: Optional[MessageType] = None) -> kiwipy.Future: | |||
| def kill_process(self, pid: 'PID_TYPE', msg_text: Optional[str] = None, force_kill: bool = False) -> kiwipy.Future: | |||
There was a problem hiding this comment.
I'd say, let's add this flag only in the other PR #288
Which is specifically for force_kill feature, and offers more changes.
I though the scope of the current PR, was to implement message interface, only.
There was a problem hiding this comment.
This I agree, I'll remove it and leave it for your PR.
src/plumpy/processes.py
Outdated
| ) | ||
|
|
||
| # If we get a message we recognise then action it, otherwise ignore | ||
| if subject == process_comms.Intent.PLAY: | ||
| return self._schedule_rpc(self.play) | ||
| if subject == process_comms.Intent.PAUSE: | ||
| return self._schedule_rpc(self.pause, msg=body) | ||
| return self._schedule_rpc(self.pause, msg_text=msg.get(process_comms.MESSAGE_KEY, None)) |
There was a problem hiding this comment.
I found this change a bit hard to read & understand.
What does it do, and this has to change?
There was a problem hiding this comment.
This is actually similar to the _perform_action of aiida-core, here the msg_text is passing as the parameter of self.pause. Together they form a callback.
I can do functools.partial(self.pause, msg_text=''), do you think that is better?
Here for broadcast_receive and message_receieve the massege passed in should be a message as a whole, it makes less sense to accept msg_text and other argument passing to command specifically.
| @@ -1125,7 +1131,7 @@ def _create_interrupt_action(self, exception: process_states.Interruption) -> fu | |||
|
|
|||
| """ | |||
| if isinstance(exception, process_states.PauseInterruption): | |||
| do_pause = functools.partial(self._do_pause, str(exception)) | |||
| do_pause = functools.partial(self._do_pause, exception.msg) | |||
There was a problem hiding this comment.
Just to add a comment that exception.msg is an attribute of PauseInterruption of type MessageType
tests/test_processes.py
Outdated
| self.assertTrue(proc.killed()) | ||
| self.assertEqual(proc.killed_msg(), msg) | ||
| self.assertEqual(proc.killed_msg()[MESSAGE_KEY], msg_text) |
There was a problem hiding this comment.
As we discussed in the office, let's make it this way:
| self.assertEqual(proc.killed_msg()[MESSAGE_KEY], msg_text) | |
| self.assertEqual(proc.killed_msg()[MessageBuilder.MESSAGE_TEXT_KEY], msg_text) |
Amend for message build, now also construct message for pause operations. See the changes required in aiida-core aiidateam/aiida-core#6668. Tests are passed.