WIP: Implement option to force kill #288
Conversation
Do not peform stepping when force_kill command is send. This avoids getting stuck in the kill callback.
There was a problem hiding this comment.
Thanks @agoscinski . Apart from one comment on the implementation, could you please add a unit test for this?
| @@ -1131,6 +1131,8 @@ def kill(self, msg: Union[str, None] = None) -> Union[bool, asyncio.Future]: | |||
| Kill the process | |||
| :param msg: An optional kill message | |||
| """ | |||
| force_kill = isinstance(msg, str) and '-F' in msg | |||
There was a problem hiding this comment.
Not a big fan of this approach. Why not add a force_kill: bool = False argument to the kill method. From aiida-core you can then pass that in the Communicator.rpc_send method
There was a problem hiding this comment.
Thanks a lot @sphuber.
Yes yes, I thought about it, but then we have to modify many functions to pass force_kill hand in hand or even to be compatible with dict.
So far, apparently in plumpy the msg is expected to be a str, if I'm not mistaken.
I tried to keep changes minimal.. although maybe not the best approach
There was a problem hiding this comment.
I think @sphuber has a good point. If the message content is checked inside the implementation, there is no way for user to know how to call it from looking at the function signature.
It is the function:
_perform_actions(processes, controller.kill_process, 'kill', 'killing', timeout, wait, msg=message)who send the rpc message, I think it is even better to change it to, and then can also pass the force_kill
_perform_actions(processes, functool.partial(controller.kill_process, msg=message), 'kill', 'killing', timeout, wait)There was a problem hiding this comment.
In #291, I try to make massage not a global variable so it is able to carry more information. After that it will be straight forward to have force option for the kill method.
Discuss with @khsrali offline, he prefer go with the current change and I'll open an amend PR after #291.
@khsrali Just add unit test for this?
There was a problem hiding this comment.
What is the point of merging this if we agree it is not a good design and we plan on changing it straight after anyway?
There was a problem hiding this comment.
What is the point of merging this if we agree it is not a good design and we plan on changing it straight after anyway?
Well, it would deliver faster to users.
There was a problem hiding this comment.
Well, it would deliver faster to users.
I may not agree with this.
Finding workarounds should not be the solution to get features fast deliver to users. The fast deliver is based on developers can quickly reach the agreement. In the case of this PR, the agreement is I'll do refactoring before or an amend fix after, on the message encapsulating part.
Do not peform stepping when
force_killcommand is send. This avoids getting stuck in the kill callback.This is part of a solution for issue aiidateam/aiida-core#6524 and goes together with the PR in aiidateam/aiida-core#6575
Co-author @khsrali