-
Notifications
You must be signed in to change notification settings - Fork 188
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
Deprecate methods that refer to a computer's label as name #4309
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,16 +9,16 @@ | |
########################################################################### | ||
# pylint: disable=invalid-name,too-many-statements,too-many-branches | ||
"""`verdi computer` command.""" | ||
|
||
from functools import partial | ||
|
||
import click | ||
import tabulate | ||
|
||
from aiida.cmdline.commands.cmd_verdi import verdi | ||
from aiida.cmdline.params import options, arguments | ||
from aiida.cmdline.params.options.commands import computer as options_computer | ||
from aiida.cmdline.utils import echo | ||
from aiida.cmdline.utils.decorators import with_dbenv | ||
from aiida.cmdline.utils.decorators import with_dbenv, deprecated_command | ||
from aiida.cmdline.utils.multi_line_input import ensure_scripts | ||
from aiida.common.exceptions import ValidationError, InputValidationError | ||
from aiida.plugins.entry_point import get_entry_points | ||
|
@@ -260,10 +260,10 @@ def computer_setup(ctx, non_interactive, **kwargs): | |
except ValidationError as err: | ||
echo.echo_critical('unable to store the computer: {}. Exiting...'.format(err)) | ||
else: | ||
echo.echo_success('Computer<{}> {} created'.format(computer.pk, computer.name)) | ||
echo.echo_success('Computer<{}> {} created'.format(computer.pk, computer.label)) | ||
|
||
echo.echo_info('Note: before the computer can be used, it has to be configured with the command:') | ||
echo.echo_info(' verdi computer configure {} {}'.format(computer.get_transport_type(), computer.name)) | ||
echo.echo_info(' verdi computer configure {} {}'.format(computer.transport_type, computer.label)) | ||
|
||
|
||
@verdi_computer.command('duplicate') | ||
|
@@ -316,20 +316,20 @@ def computer_duplicate(ctx, computer, non_interactive, **kwargs): | |
except (ComputerBuilder.ComputerValidationError, ValidationError) as e: | ||
echo.echo_critical('{}: {}'.format(type(e).__name__, e)) | ||
else: | ||
echo.echo_success('stored computer {}<{}>'.format(computer.name, computer.pk)) | ||
echo.echo_success('stored computer {}<{}>'.format(computer.label, computer.pk)) | ||
|
||
try: | ||
computer.store() | ||
except ValidationError as err: | ||
echo.echo_critical('unable to store the computer: {}. Exiting...'.format(err)) | ||
else: | ||
echo.echo_success('Computer<{}> {} created'.format(computer.pk, computer.name)) | ||
echo.echo_success('Computer<{}> {} created'.format(computer.pk, computer.label)) | ||
|
||
is_configured = computer.is_user_configured(orm.User.objects.get_default()) | ||
|
||
if not is_configured: | ||
echo.echo_info('Note: before the computer can be used, it has to be configured with the command:') | ||
echo.echo_info(' verdi computer configure {} {}'.format(computer.get_transport_type(), computer.name)) | ||
echo.echo_info(' verdi computer configure {} {}'.format(computer.transport_type, computer.label)) | ||
|
||
|
||
@verdi_computer.command('enable') | ||
|
@@ -344,15 +344,15 @@ def computer_enable(computer, user): | |
authinfo = computer.get_authinfo(user) | ||
except NotExistent: | ||
echo.echo_critical( | ||
"User with email '{}' is not configured for computer '{}' yet.".format(user.email, computer.name) | ||
"User with email '{}' is not configured for computer '{}' yet.".format(user.email, computer.label) | ||
) | ||
|
||
if not authinfo.enabled: | ||
authinfo.enabled = True | ||
echo.echo_info("Computer '{}' enabled for user {}.".format(computer.name, user.get_full_name())) | ||
echo.echo_info("Computer '{}' enabled for user {}.".format(computer.label, user.get_full_name())) | ||
else: | ||
echo.echo_info( | ||
"Computer '{}' was already enabled for user {} {}.".format(computer.name, user.first_name, user.last_name) | ||
"Computer '{}' was already enabled for user {} {}.".format(computer.label, user.first_name, user.last_name) | ||
) | ||
|
||
|
||
|
@@ -370,15 +370,17 @@ def computer_disable(computer, user): | |
authinfo = computer.get_authinfo(user) | ||
except NotExistent: | ||
echo.echo_critical( | ||
"User with email '{}' is not configured for computer '{}' yet.".format(user.email, computer.name) | ||
"User with email '{}' is not configured for computer '{}' yet.".format(user.email, computer.label) | ||
) | ||
|
||
if authinfo.enabled: | ||
authinfo.enabled = False | ||
echo.echo_info("Computer '{}' disabled for user {}.".format(computer.name, user.get_full_name())) | ||
echo.echo_info("Computer '{}' disabled for user {}.".format(computer.label, user.get_full_name())) | ||
else: | ||
echo.echo_info( | ||
"Computer '{}' was already disabled for user {} {}.".format(computer.name, user.first_name, user.last_name) | ||
"Computer '{}' was already disabled for user {} {}.".format( | ||
computer.label, user.first_name, user.last_name | ||
) | ||
) | ||
|
||
|
||
|
@@ -400,7 +402,7 @@ def computer_list(all_entries, raw): | |
if not computers: | ||
echo.echo_info("No computers configured yet. Use 'verdi computer setup'") | ||
|
||
sort = lambda computer: computer.name | ||
sort = lambda computer: computer.label | ||
highlight = lambda comp: comp.is_user_configured(user) and comp.is_user_enabled(user) | ||
hide = lambda comp: not (comp.is_user_configured(user) and comp.is_user_enabled(user)) and not all_entries | ||
echo.echo_formatted_list(computers, ['name'], sort=sort, highlight=highlight, hide=hide) | ||
|
@@ -411,36 +413,57 @@ def computer_list(all_entries, raw): | |
@with_dbenv() | ||
def computer_show(computer): | ||
"""Show detailed information for a computer.""" | ||
echo.echo(computer.full_text_info) | ||
table = [] | ||
table.append(['Label', computer.label]) | ||
table.append(['PK', computer.pk]) | ||
table.append(['UUID', computer.uuid]) | ||
table.append(['Description', computer.description]) | ||
table.append(['Hostname', computer.hostname]) | ||
table.append(['Transport type', computer.transport_type]) | ||
table.append(['Scheduler type', computer.scheduler_type]) | ||
table.append(['Work directory', computer.get_workdir()]) | ||
table.append(['Shebang', computer.get_shebang()]) | ||
table.append(['Mpirun command', ' '.join(computer.get_mpirun_command())]) | ||
table.append(['Prepend text', computer.get_prepend_text()]) | ||
table.append(['Append text', computer.get_append_text()]) | ||
echo.echo(tabulate.tabulate(table)) | ||
|
||
|
||
@verdi_computer.command('rename') | ||
@arguments.COMPUTER() | ||
@arguments.LABEL('NEW_NAME') | ||
@deprecated_command("This command has been deprecated. Please use 'verdi computer relabel' instead.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely unrelated, but should we maybe have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would certainly be an option, although low priority I would say. Usually you copy this line from some other place anyway so it is already there. |
||
@click.pass_context | ||
@with_dbenv() | ||
def computer_rename(computer, new_name): | ||
def computer_rename(ctx, computer, new_name): | ||
"""Rename a computer.""" | ||
ctx.invoke(computer_relabel, computer=computer, label=new_name) | ||
|
||
|
||
@verdi_computer.command('relabel') | ||
@arguments.COMPUTER() | ||
@arguments.LABEL('LABEL') | ||
@with_dbenv() | ||
def computer_relabel(computer, label): | ||
"""Relabel a computer.""" | ||
from aiida.common.exceptions import UniquenessError | ||
|
||
old_name = computer.get_name() | ||
old_label = computer.label | ||
|
||
if old_name == new_name: | ||
echo.echo_critical('The old and new names are the same.') | ||
if old_label == label: | ||
echo.echo_critical('The old and new labels are the same.') | ||
|
||
try: | ||
computer.set_name(new_name) | ||
computer.label = label | ||
computer.store() | ||
except ValidationError as error: | ||
echo.echo_critical('Invalid input! {}'.format(error)) | ||
except UniquenessError as error: | ||
echo.echo_critical( | ||
'Uniqueness error encountered! Probably a ' | ||
"computer with name '{}' already exists" | ||
''.format(new_name) | ||
"Uniqueness error encountered! Probably a computer with label '{}' already exists: {}".format(label, error) | ||
) | ||
echo.echo_critical('(Message was: {})'.format(error)) | ||
|
||
echo.echo_success("Computer '{}' renamed to '{}'".format(old_name, new_name)) | ||
echo.echo_success("Computer '{}' relabeled to '{}'".format(old_label, label)) | ||
|
||
|
||
@verdi_computer.command('test') | ||
|
@@ -472,15 +495,15 @@ def computer_test(user, print_traceback, computer): | |
if user is None: | ||
user = orm.User.objects.get_default() | ||
|
||
echo.echo_info('Testing computer<{}> for user<{}>...'.format(computer.name, user.email)) | ||
echo.echo_info('Testing computer<{}> for user<{}>...'.format(computer.label, user.email)) | ||
|
||
try: | ||
authinfo = computer.get_authinfo(user) | ||
except NotExistent: | ||
echo.echo_critical('Computer<{}> is not yet configured for user<{}>'.format(computer.name, user.email)) | ||
echo.echo_critical('Computer<{}> is not yet configured for user<{}>'.format(computer.label, user.email)) | ||
|
||
if not authinfo.enabled: | ||
echo.echo_warning('Computer<{}> is disabled for user<{}>'.format(computer.name, user.email)) | ||
echo.echo_warning('Computer<{}> is disabled for user<{}>'.format(computer.label, user.email)) | ||
click.confirm('Do you really want to test it?', abort=True) | ||
|
||
scheduler = authinfo.computer.get_scheduler() | ||
|
@@ -568,14 +591,14 @@ def computer_delete(computer): | |
from aiida.common.exceptions import InvalidOperation | ||
from aiida import orm | ||
|
||
compname = computer.name | ||
label = computer.label | ||
|
||
try: | ||
orm.Computer.objects.delete(computer.id) | ||
except InvalidOperation as error: | ||
echo.echo_critical(str(error)) | ||
|
||
echo.echo_success("Computer '{}' deleted.".format(compname)) | ||
echo.echo_success("Computer '{}' deleted.".format(label)) | ||
|
||
|
||
@verdi_computer.group('configure') | ||
|
@@ -594,12 +617,11 @@ def computer_configure(): | |
@arguments.COMPUTER() | ||
def computer_config_show(computer, user, defaults, as_option_string): | ||
"""Show the current configuration for a computer.""" | ||
import tabulate | ||
from aiida.common.escaping import escape_for_bash | ||
|
||
transport_cls = computer.get_transport_class() | ||
option_list = [ | ||
param for param in transport_cli.create_configure_cmd(computer.get_transport_type()).params | ||
param for param in transport_cli.create_configure_cmd(computer.transport_type).params | ||
if isinstance(param, click.core.Option) | ||
] | ||
option_list = [option for option in option_list if option.name in transport_cls.get_valid_auth_params()] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, can I ask what is the intended purpose of the commands
show
andcomputer_show
modified in this PR? Because I'm not sure I understand why you are removing the generalget_full_text_info
method to implement two relatively similar versions directly into each of those commands.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that one is
verdi code show
and the otherverdi computer show
. I removed the methods from the classes because they don't really belong there. Especially theComputer
one was bad since it was baking in the formatting. It was returning a string fully formatted. This is really inflexible as the caller cannot do anything to modify it. TheCode
one is already slightly better since it simply returned a list of tuples. Still I think deciding what information should be displayed is still very situation dependent and so there is no need to have a dedicated method for this. It was no wonder that the methods were also just used in one place, namely theshow
CLI commands. Might as well put that logic there so it has all the freedom it needs to choose what and how to display the informationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, I didn't notice these were methods of different classes. Thanks!