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

Set default for the safe_interval option of verdi computer configure #3590

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 29, 2019

Fixes #2880

The safe_interval is a common option for all Transport types but its
default value is class dependent. Since it is a common option it was
defined in the Transport base class, but since it is defined as a
class attribute, the default cannot be set yet. Keeping this design, the
only option to insert the class specific default is in auth_options
the class property that returns the options for the CLI. By adding the
default here, verdi computer configure local localhost -n will now
work without prompting for its only option safe_interval as it will
simply use the default.

@sphuber sphuber requested a review from ltalirz November 29, 2019 13:53
@sphuber sphuber force-pushed the fix_2880_localhost_configure_safe_interval branch 3 times, most recently from 04455b9 to fb15b22 Compare November 29, 2019 17: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.

thanks a lot for the fix @sphuber
man, this is nasty code to fix...

Given that the ..._options_... lists already contain click-specific code, it seems to me that rather than trying to synthesize the click commands from some custom format, the much more straightforward solution would be to define the click commands explicitly, and to extract from them whatever is needed through inspection:

import click

@click.command()
@click.option('--count', default=1, help='Number of greetings.')
@click.option('--name', prompt='Your name',
              help='The person to greet.')
def hello(count, name):
    """Simple program that greets NAME for a total of COUNT times."""
    for x in range(count):
        click.echo('Hello %s!' % name)

print(hello.params)

Perhaps we should open an issue for this...

aiida/transports/cli.py Outdated Show resolved Hide resolved
# The common auth options are currently defined as class members, but the default for `safe_interval` is sub
# class specific. With the current design the default cannot already be specified directly but has to be added
# manually here.
for option in cls._common_auth_options:
Copy link
Member

Choose a reason for hiding this comment

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

hm, nasty...
how about making _common_auth_options and _valid_auth_options OrderedDicts themselves. that would make accessing them easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will make it impossible to set the correct default though, see the comment.

Copy link
Member

Choose a reason for hiding this comment

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

hm... I don't quite follow. How does _common_auth_options being an OrderedDict instead of a list of tuples change this?
Anyhow it would just be a minor improvement, not important.

The `safe_interval` is a common option for all `Transport` types but its
default value is class dependent. Since it is a common option it was
defined in the `Transport` base class, but since it is defined as a
class attribute, the default cannot be set yet. Keeping this design, the
only option to insert the class specific default is in `auth_options`
the class property that returns the options for the CLI. By adding the
default here, `verdi computer configure local localhost -n` will now
work without prompting for its only option `safe_interval` as it will
simply use the default.
@sphuber sphuber force-pushed the fix_2880_localhost_configure_safe_interval branch from fb15b22 to 34bc1b9 Compare November 30, 2019 14:51
@sphuber
Copy link
Contributor Author

sphuber commented Nov 30, 2019

man, this is nasty code to fix...

Given that the ..._options_... lists already contain click-specific code, it seems to me that rather than trying to synthesize the click commands from some custom format, the much more

That will remove the functionality that the verdi computer configure commands are created automatically for all registered transports. Once you remove this, people will have to define these commands manually and explicitly.

@ltalirz
Copy link
Member

ltalirz commented Nov 30, 2019

That will remove the functionality that the verdi computer configure commands are created automatically for all registered transports. Once you remove this, people will have to define these commands manually and explicitly.

Right - it would mean you need to write the click command and the transport class (that inspects it) together, which might be less effort than writing just the transport class with the current machinery.

Anyhow, it would also not be ideal.
For me this PR is good to merge!

@sphuber sphuber merged commit 787afd9 into aiidateam:develop Dec 1, 2019
@sphuber sphuber deleted the fix_2880_localhost_configure_safe_interval branch December 1, 2019 10:09
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.

connection cooldown time for local computers?
2 participants