Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config option daemon.default_workers and daemon.worker_process_slots #3949

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 20, 2020

Fixes #3677

This option will determine the default number of workers that are
started by the verdi daemon start command. Specifying an explicit
value as argument will of course still override this.

This option is used in the construction of the RabbitMQ communicator and
will determine the maximum number of process tasks that any one daemon
runner can operate on concurrently. If this number is set too high, the
runner can get overwhelmed as there is no logic for it to prioritize
finishing active processes over taking on new processes that have
arrived in the process task queue.

@sphuber sphuber requested a review from ltalirz April 20, 2020 20:12
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #3949 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3949   +/-   ##
========================================
  Coverage    78.31%   78.31%           
========================================
  Files          461      461           
  Lines        34079    34082    +3     
========================================
+ Hits         26688    26693    +5     
+ Misses        7391     7389    -2     
Flag Coverage Δ
#django 70.36% <100.00%> (+<0.01%) ⬆️
#sqlalchemy 71.21% <100.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
aiida/manage/external/rmq.py 67.46% <ø> (+0.80%) ⬆️
aiida/cmdline/commands/cmd_daemon.py 56.14% <100.00%> (+0.51%) ⬆️
aiida/cmdline/utils/common.py 65.55% <100.00%> (+0.12%) ⬆️
aiida/manage/configuration/options.py 75.00% <100.00%> (+1.31%) ⬆️
aiida/manage/manager.py 95.39% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0936512...7fd1b81. Read the comment docs.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot @sphuber , looks great!

a few small questions

'valid_type': 'int',
'valid_values': None,
'default': DEFAULT_DAEMON_WORKERS,
'description': 'The default number of workers to be launched by `verdi daemon start`',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to understand - e.g. if I use incr does this value change?
In which sense is it a default rather than the actual number of daemon workers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verdi daemon incr is an independent command. It still takes an argument to tell how many workers should be added, verdi daemon incr N will add N workers with default being 1. The new option only applies how many workers are started automatically when invoking verdi daemon start. Already you can do verdi daemon start 10 to start 10 workers straight away, but now you can configure it to have it start 10 by default. Saves an entire three key strokes ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I just wanted to make sure.
I think some users might assume that if you do verdi daemon incr 1 and then restart the daemon that you would keep the increased number of workers rather than resetting to this default.

It might make sense to mention explicitly in the docstring of verdi daemon incr that these changes are reset whenever you stop the daemon, and that you should use this new config option to make permanent changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit more subtle than that even. Doing verdi daemon restart will restart the workers but not the daemon itself, i.e. you will keep how every many workers that were running before the restart command. Doing verdi daemon restart --reset literally does verdi daemon stop && verdi daemon start so there it will start whatever is the default for verdi daemon start, which can now be configured through this new option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already explained in the docstring of verdi daemon restart:

    By default will only reset the workers of the running daemon. After the restart the same amount of workers will be
    running. If the `--reset` flag is passed, however, the full circus daemon will be stopped and restarted with just
    a single worker.

I just noticed that we just have to update the last bit with respect to the new default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the docstrings of verdi daemon start and verdi daemon restart in the first commit to reflect the new behavior and give more detail

aiida/manage/manager.py Show resolved Hide resolved
aiida/manage/manager.py Outdated Show resolved Hide resolved
@sphuber sphuber force-pushed the feature/3677/configurable-task-prefetch-count branch from 35393a9 to 90889c3 Compare April 21, 2020 11:42
sphuber added 2 commits April 21, 2020 14:02
This option will determine the default number of workers that are
started by the `verdi daemon start` command. Specifying an explicit
value as argument will of course still override this.
This option is used in the construction of the RabbitMQ communicator and
will determine the maximum number of process tasks that any one daemon
runner can operate on concurrently. If this number is set too high, the
runner can get overwhelmed as there is no logic for it to prioritize
finishing active processes over taking on new processes that have
arrived in the process task queue.
@sphuber sphuber force-pushed the feature/3677/configurable-task-prefetch-count branch from 90889c3 to 7fd1b81 Compare April 21, 2020 12:04
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

@sphuber
Copy link
Contributor Author

sphuber commented Apr 21, 2020

Thanks for the reviews @ltalirz !

@sphuber sphuber merged commit 6aa6994 into aiidateam:develop Apr 21, 2020
@sphuber sphuber deleted the feature/3677/configurable-task-prefetch-count branch April 21, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make RMQ prefetch count a profile option
2 participants