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

Forward "verdi code delete" to "verdi node delete" #3546

Merged
merged 4 commits into from
Nov 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions aiida/backends/djsite/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,16 +162,23 @@ def check_schema_version(profile_name):
set_db_schema_version(code_schema_version)
db_schema_version = get_db_schema_version()

if code_schema_version != db_schema_version:
if code_schema_version > db_schema_version:
Copy link
Contributor

Choose a reason for hiding this comment

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

This code also exists for SqlAlchemy, maybe you want to update it there as well. Although this is not as easy as there you cannot simply compare revisions by their value. Anyway this code is touched heavily by PR #3582

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so how would one do it?
If there's no good way to do it, I guess we can leave the sqlalchemy version untouched.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine for now, but these changes will probably become irrelevant with PR #3582 . But we can merge this now.

raise ConfigurationError(
'Database schema version {} is outdated compared to the code schema version {}\n'
'Before you upgrade, make sure all calculations and workflows have finished running.\n'
'If this is not the case, revert the code to the previous version and finish them first.\n'
'To migrate the database to the current version, run the following commands:'
'Database schema version {} is outdated compared to the code schema version {}.\n'
'Note: Have all calculations and workflows have finished running? '
'If not, revert the code to the previous version and let them finish before upgrading.\n'
'To migrate the database to the current code version, run the following commands:'
'\n verdi -p {} daemon stop\n verdi -p {} database migrate'.format(
db_schema_version, code_schema_version, profile_name, profile_name
)
)
elif code_schema_version < db_schema_version:
raise ConfigurationError(
'Database schema version {} is newer than code schema version {}.\n'
'You cannot use an outdated code with a newer database. Please upgrade.'.format(
db_schema_version, code_schema_version
)
)


def set_db_schema_version(version):
Expand Down
11 changes: 10 additions & 1 deletion aiida/backends/tests/cmdline/commands/test_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,23 @@ def test_relabel_code_full_bad(self):
result = self.cli_runner.invoke(relabel, [str(self.code.pk), 'new_code@otherstuff'])
self.assertIsNotNone(result.exception)

def test_delete_one(self):
def test_code_delete_one(self):
result = self.cli_runner.invoke(delete, [str(self.code.pk)])
self.assertIsNone(result.exception, result.output)

with self.assertRaises(NotExistent):
from aiida.orm import Code
Code.get_from_string('code')

def test_code_delete_one_force(self):
sphuber marked this conversation as resolved.
Show resolved Hide resolved
result = self.cli_runner.invoke(delete, [str(self.code.pk), '--force'])
self.assertIsNone(result.exception, result.output)

with self.assertRaises(NotExistent):
from aiida.orm import Code
Code.get_from_string('code')


def test_code_list(self):
# set up second code 'code2'
from aiida.orm import Code
Expand Down
28 changes: 14 additions & 14 deletions aiida/cmdline/commands/cmd_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,25 +172,25 @@ def show(code, verbose):

@verdi_code.command()
@arguments.CODES()
@options.VERBOSE()
@options.DRY_RUN()
@options.FORCE()
@with_dbenv()
def delete(codes):
def delete(codes, verbose, dry_run, force):
"""Delete a code.

Note that it is possible to delete a code only if it has not yet been used
as an input of a calculation, i.e., if it does not have outgoing links.
Note that codes are part of the data provenance, and deleting a code will delete all calculations using it.
"""
from aiida.common.exceptions import InvalidOperation
from aiida.orm import Node
from aiida.manage.database.delete.nodes import delete_nodes

for code in codes:
try:
pk = code.pk
full_label = code.full_label
Node.objects.delete(pk) # pylint: disable=no-member
except InvalidOperation as exception:
echo.echo_error(str(exception))
else:
echo.echo_success('Code<{}> {} deleted'.format(pk, full_label))
verbosity = 1
if force:
verbosity = 0
elif verbose:
verbosity = 2

node_pks_to_delete = [code.pk for code in codes]
delete_nodes(node_pks_to_delete, dry_run=dry_run, verbosity=verbosity, force=force)
Copy link
Contributor

Choose a reason for hiding this comment

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

What will the message now be here for a code with calculations and force=False? I guess since it pipes through to delete_nodes it will not explicitly mention code or its calculations but just nodes in general. Would it maybe be better to do the check in this command and raise a critical error unless force=True. Then the message can be made more specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

An earlier version of the PR did exactly this - but then I also agree with @ramirezfranciscof that I don't really see a reason to be more strict with codes than with other nodes.
When one calls verdi code delete ... without --force, it will simply prompt yes/no if there are other nodes attached.
To me this seems fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough



@verdi_code.command()
Expand Down