CLI: Add --timeout option for verdi daemon start and verdi daemon stop

[AhmedBasem20]

Fixes #5637

Made the timeouts for DaemonClient configurable via two CLI options for verdi daemon start and verdi daemon stop.

I'm not sure about the testing approach here so I'll be waiting for the review.

Thanks.

[unkcpz]

Thanks @AhmedBasem20 for the contribution (again!)

However, I think the implementation here requires more discussion. From my experience as a user, I never encounter issue with verdi daemon start, and I think the start daemon not actually waiting for something and block there.
For the verdi daemon stop, the timeout defined in the CLI only happened after the timeout of circus client operations are finished. I am not very sure about the code path but when I have processes that need to be closed the verdi daemon stop command is running longer than the default timeout (5 seconds) of stop_daemon (see:

aiida-core/aiida/engine/daemon/client.py

Line 462 in 0bc8ef0

def stop_daemon(self, wait: bool = True, timeout: int = 5) -> JsonDictType:

).
As for the timeout of daemon_client, it is already a configurable parameter get by config.get_option('daemon.timeout') (see:

aiida-core/aiida/engine/daemon/client.py

Line 86 in 0bc8ef0

self._DAEMON_TIMEOUT: int = config.get_option('daemon.timeout') # pylint: disable=invalid-name

)

My question here for @sphuber is whether the timeout of stop_daemon starts ticking after the circus call is finished which means the timeout of daemon.timeout is hinted.

[AhmedBasem20]

Hi @unkcpz thanks.

I added an option for verdi daemon start too based on the discussion of the previous PR here: #5631 (comment)
We may discuss it more to see if it's a common use case or should be removed.

[sphuber]

Thanks @AhmedBasem20 and @unkcpz . To provide some context, the stop_daemon and start_daemon commands were added in #5631 such that the Python API also had a way of starting/stopping the daemon just as verdi provides. The idea was that when your run DaemonClient.start_daemon, you want the response to be whether the daemon actually started successfully. Since it is launched by calling a subprocess, the only way of doing this was by periodically calling the status of the daemon, and so this required a timeout. Note that in this case, we are not going through the CircusClient, because there isn't a circus daemon running yet to talk to.

Now, for stop_daemon it is actually different becaause there the CircusClient is actually used, which indeed already has a timeout that is taken from daemon.timeout. For consistency, I also added an explicit check to make sure the daemon was properly stopped, for which the new timeout argument was introduced. Now, to be honest, I am not sure whether the call to the CircusClient:

command = {'command': 'quit', 'properties': {'waiting': wait}}
result = self.call_client(command)

already handles this for us, i.e., does this only return when the daemon is actually stopped (or otherwise it times out). If that is the case, then our second layer of checking is not even necessary and we may best just get rid of it. And in that case, we should probably also just use the daemon.timeout for the start_daemon timeout and not add a new argument.

I am not sure how to test the behavior of the CircusClient though. Anyway, I think the conclusion is that before exposing these timeout method arguments to the CLI, we should first figure out how things actually work. Maybe we can best put this @AhmedBasem20 on hold. Of course, if you feel up to it, you are very welcome to try and figure out how the CircusClient actually works and update the DaemonClient if we figure out it does things unnecessarily.

[AhmedBasem20]

Thanks @sphuber. I tried to figure out the behavior of the CircusClient but this issue seems to be more complicated than I thought. So I'll better leave it for now.

[ltalirz]

@sphuber is this PR superseded by your recent work?

[sphuber]

@sphuber is this PR superseded by your recent work?

Not quite yet, but it is a necessary pre-cursor to make the code clearer as to which timeouts there are and which need to be exposed on the CLI. I will come back to this, once those other PRs are merged but don't have the time to finalize them now

[sphuber]

Thanks for the PR @AhmedBasem20 . Since the code has changed after fixing the DaemonClient, there were conflicts here. Since we are releasing v2.3 early next week and so there is not much time, I went ahead and created #5966 to replace this PR. Hope you don't mind.

[AhmedBasem20]

Thank you for getting back @sphuber. No problems at all. It's great that you figured out and solved that issue in the last two PRs!