Message interface is hidden inside function call

[unkcpz]

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.

[khsrali]

Thanks @unkcpz , I asked some question, just to understand.

[khsrali]

I don't understand this change 🤔 MESSAGE_KEY is imported and passed as key, why is that?

[unkcpz]

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.

[khsrali]

As we discussed in the office, let's make it this way:

Suggested change

	self.assertEqual(proc.killed_msg()[MESSAGE_KEY], msg_text)
	self.assertEqual(proc.killed_msg()[MessageBuilder.MESSAGE_TEXT_KEY], msg_text)

[unkcpz]

@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']

[khsrali]

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']

[unkcpz]

msg = proc.killed_msg()
msg_text = msg['MESSAGE_TEXT']

I think this is not good since the typo in the str won't be checked by the pre-commit or modern IDE. Meanwhile, we did struggle a lot by that the exception from plumpy is not well propagated to the aiida log which makes such error even hard to find and debug. Usually such exception can cause the test in aiida-core hang up rather than raising exceptions.

msg = proc.killed_msg()
msg_text = msg[mb.MESSAGE_TEXT_KEY]

mb can be quite confuse, it looks more like megabytes from first glimpse.

The proper solution will be defining in plumpy.process_comms a standard message protocol and hasing message defined as a dataclass then used in plumpy and se/de -serialize the message through for example msgpack. It is a generic pattern, which can be found such as in redis or bit-torrent.
But change the message to dataclass require also change the de/se function which is beyond the scope of this PR.

TBH, I think this is a very small problem and only the matter of taste. As you can see from https://github.com/aiidateam/aiida-core/pull/6668/files, the MESSAGE_TEXT_KEY is not used there but only in plumpy, it defined as a part of message protocol of current design (which using dict as the message body).

[khsrali]

Thank you @unkcpz , just minor things as we discussed

[khsrali]

As we discussed in the office, let's make it this way:

Suggested change

	self.assertEqual(proc.killed_msg()[MESSAGE_KEY], msg_text)
	self.assertEqual(proc.killed_msg()[MessageBuilder.MESSAGE_TEXT_KEY], msg_text)