search

aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
435 stars 188 forks source link

`verdi daemon restart` ignores change of daemon.default_workers (and possibly other values) #4948

Closed dev-zero closed 7 months ago

dev-zero commented 3 years ago

Describe the bug

$ verdi daemon status
Profile: default
Daemon is running as PID 214995 since 2021-05-18 08:35:19
Active workers [1]:
   PID    MEM %    CPU %  started
------  -------  -------  -------------------
214999    1.835        0  2021-05-18 08:35:20
Use verdi daemon [incr | decr] [num] to increase / decrease the amount of workers
$ verdi config set daemon.default_workers 4
Success: 'daemon.default_workers' set to 4 for 'default' profile
$ verdi daemon restart
Restarting the daemon... OK
$ verdi daemon status
Profile: default
Daemon is running as PID 214995 since 2021-05-18 08:35:19
Active workers [1]:
   PID    MEM %    CPU %  started
------  -------  -------  -------------------
215162    1.104        0  2021-05-18 08:36:46
Use verdi daemon [incr | decr] [num] to increase / decrease the amount of workers

Expected behavior

After the restart the new number of workers should be used

Your environment

ltalirz commented 3 years ago

This is indeed unexpected - @sphuber do you agree?

ltalirz commented 3 years ago

@dev-zero what happens when you stop and start again?

dev-zero commented 3 years ago

it comes up with the correct number of workers

sphuber commented 3 years ago

This is indeed unexpected - @sphuber do you agree?

Not necessarily. verdi daemon restart is a soft restart and only restarts the workers, not the circus daemonizer. That means all settings remain unchanged. For that you need a full restart through verdi daemon restart --reset which is equivalent to verdi daemon stop && verdi daemon start.

If this is unclear, what we could do is making verdi daemon restart do the full reset and then replace --reset flag with something else (say --workers or --soft) and have that just restart the workers. Reason I did it the current way is that often one wants to just restart the workers and that is a lot faster than fully restarting the entire daemon

dev-zero commented 3 years ago

from Linux world (systemd and what not) I am used to restart being equivalent to a stop and start

ltalirz commented 3 years ago

The time difference is indeed significant, so I understand @sphuber 's original rationale.

$ time (verdi daemon stop && verdi daemon start)
Profile: nanoporous_pipeline
Waiting for the daemon to shut down... OK
Starting the daemon... RUNNING

real    0m5.027s
user    0m3.386s
sys 0m2.615s
$ time (verdi daemon restart)
Restarting the daemon... OK

real    0m1.572s
user    0m1.224s
sys 0m0.838s

At the same time, in typical operation restarting the daemon is a rare event, i.e. 4 seconds more or less does not make a world of difference and I would prefer simplicity of use over "performance" in this case (as the default behavior).

I can certainly see users be confused by restart not being equivalent to stop + start (and e.g. I implicitly assumed it myself in the proposed text for https://github.com/aiidateam/aiida-core/pull/4902/files).

If this is unclear, what we could do is making verdi daemon restart do the full reset and then replace --reset flag with something else (say --workers or --soft) and have that just restart the workers.

I would vote for this solution (--soft).

giovannipizzi commented 8 months ago

Since we often anyway recommend verdi daemon restart --reset, shall we implement this change (and go with a full restart by default, and --soft as an option)? @sphuber @khsrali @GeigerJ2 - if we agree, it's an easy change

sphuber commented 8 months ago

Yeah, I think that would make sense. We can simply deprecate --reset and say that is now the default. I am not even sure that we would need the other option for now. If they want to restart the workers, they can just restart the entire daemon. The only downside is that it takes a bit longer, but I don't think that will ever be a problem, especially since it won't be clear to the user anyway when they can do with just resetting the workers and not the entire daemon.

@khsrali @GeigerJ2 either of you want to pick this one up?

khsrali commented 8 months ago

yes, I agree it makes sense. It should be an easy change. I'll do it, it's a good exercise.

danielhollas commented 7 months ago

The time difference is indeed significant, so I understand @sphuber 's original rationale.

FWIW I re-did a quick n dirty timing on my machine on current main, and the soft reset now takes about 1s, while the hard reset about 1.8s so the difference seems to be much less significant. :+1: