Navigation Menu

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

Allow to specify number of workers in verdi daemon start #3001

Merged
merged 1 commit into from Jun 14, 2019

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 13, 2019

Fixes #2998

Now you can start more than just one worker when starting the daemon.

@coveralls
Copy link

coveralls commented Jun 13, 2019

Coverage Status

Coverage increased (+0.2%) to 73.203% when pulling becb8b5 on sphuber:fix_2998_verdi_daemon_start_num into ababf1c on aiidateam:develop.

@sphuber sphuber force-pushed the fix_2998_verdi_daemon_start_num branch from 63cf3a6 to 640674f Compare June 13, 2019 16:42
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.

looks all good, just one question

@sphuber sphuber force-pushed the fix_2998_verdi_daemon_start_num branch from 640674f to 5a6a373 Compare June 14, 2019 08:00
@verdi.group('daemon')
def verdi_daemon():
"""Inspect and manage the daemon."""


@verdi_daemon.command()
@click.option('--foreground', is_flag=True, help='Run in foreground.')
@click.argument('number', default=1, type=int, callback=validate_positive_non_zero_integer)
Copy link
Member

Choose a reason for hiding this comment

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

Could this be an option instead?
The command currently is verdi start-circus 12 and I think verdi start-circus --nworkers 12 (or similar) would be more self-explanatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One never calls verdi daemon start-circus it is a hidden command. But it would be verdi daemon start [NUMBER]. I chose the argument because that is inline with the already existing commands verdi daemon incr/decr [NUMBER].

@ltalirz
Copy link
Member

ltalirz commented Jun 14, 2019

Thanks, that looks already much better!

Sorry to keep nagging here - just two more questions:

  • Is there a configuration setting for the default number of workers? Or is this something one has to put every time one starts the daemon? If there is no configuration setting for this, perhaps we should add an issue for this?
  • One thing I never quite understood: Do we need to go via a hidden click command here? Can't we just spawn a process directly from a function like so?
    Anyhow, this is just out of curiosity.

@sphuber
Copy link
Contributor Author

sphuber commented Jun 14, 2019

Is there a configuration setting for the default number of workers? Or is this something one has to put every time one starts the daemon?

There isn't a configuration setting yet, as this functionality is new, so yes, for now one would always have to specify an explicit number if one wants more than the default one. I don't think it is a problem, but adding a new issue with a feature request to make this configurable would be a good idea.

Do we need to go via a hidden click command here?

It sure is not necessary, it would certainly be possible to implement it differently. I think Rico implemented the very first command this way when migrating to circus. I just kept it and especially now that it is properly hidden, I don't see any harm

@ltalirz
Copy link
Member

ltalirz commented Jun 14, 2019

Ok, then I would kindly request to

  • open an issue as discussed above
  • mention somewhere in an appropriate docstring (or in a separate issue) the alternative of going through multiprocessing

Will approve then - am offline for 2h now

@sphuber
Copy link
Contributor Author

sphuber commented Jun 14, 2019

open an issue as discussed above

Done, see #3007

mention somewhere in an appropriate docstring (or in a separate issue) the alternative of going through multiprocessing

I just realized why keeping the click route is preferable. Since we are launching a sub process, the correct profile needs to be loaded. This is currently taken care of automatically by the verdi click infrastructure that correctly parses and loads the desired profile. If we were to go through a normal function that we would call as a sub process, that function would have to provide some way to take a profile different from the default. You will basically end up implementing another command line parser.

Now you can start more than just one worker when starting the daemon.
@sphuber sphuber force-pushed the fix_2998_verdi_daemon_start_num branch from 5a6a373 to becb8b5 Compare June 14, 2019 16:33
@ltalirz ltalirz self-requested a review June 14, 2019 18:59
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 !

@sphuber sphuber merged commit 09531ad into aiidateam:develop Jun 14, 2019
@sphuber sphuber deleted the fix_2998_verdi_daemon_start_num branch June 14, 2019 20:02
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.

Allow to specify the number of workers in verdi daemon start
3 participants