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.

Sign up for GitHub

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

CLI: add the verdi database version command #4613

Merged
merged 1 commit into from
Dec 7, 2020
Merged

CLI: add the verdi database version command #4613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions aiida/cmdline/commands/cmd_database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,22 @@ def verdi_database():
"""Inspect and manage the database."""


@verdi_database.command('version')
def database_version():
"""Show the version of the database.

The database version is defined by the tuple of the schema generation and schema revision.
"""
from aiida.manage.manager import get_manager

backend_manager = get_manager().get_backend_manager()

echo.echo('Generation: ', bold=True, nl=False)
echo.echo(backend_manager.get_schema_generation_database())
echo.echo('Revision: ', bold=True, nl=False)
echo.echo(backend_manager.get_schema_version_database())


@verdi_database.command('migrate')
@options.FORCE()
def database_migrate(force):
Expand Down
9 changes: 6 additions & 3 deletions aiida/manage/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ def _load_backend(self, schema_check=True):

# Do NOT reload the backend environment if already loaded, simply reload the backend instance after
if configuration.BACKEND_UUID is None:
manager = self.get_backend_manager()
manager.load_backend_environment(profile, validate_schema=schema_check)
from aiida.backends import get_backend_manager
backend_manager = get_backend_manager(self.get_profile().database_backend)
ltalirz marked this conversation as resolved.
Show resolved Hide resolved
backend_manager.load_backend_environment(profile, validate_schema=schema_check)
configuration.BACKEND_UUID = profile.uuid

backend_type = profile.database_backend
Expand Down Expand Up @@ -123,8 +124,10 @@ def get_backend_manager(self):
:return: the database backend manager
:rtype: :class:`aiida.backend.manager.BackendManager`
"""
from aiida.backends import get_backend_manager
ltalirz marked this conversation as resolved.
Show resolved Hide resolved

if self._backend_manager is None:
from aiida.backends import get_backend_manager
self._load_backend()
Copy link
Member

Choose a reason for hiding this comment

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

get_backend_manager should load the backend?

isn't that a significant change?
should this be in this PR?

I probably just miss the reasoning for these changes, could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the changes in this file can be explained as follows, and they should answer all three of your questions. The get_backend_manager was so far not loading the backend, although that really doesn't make any sense. It is providing access to data from the database, but to do so the backend should be loaded, otherwise a connection isn't possible. This problem went unnoticed, because the BackendManager is only used in aiida.engine.utils.set_process_state_change_timestamp. By the time this gets used, the database backend will already have been loaded through another code path.

For the changes in this PR, however, I needed to get just the backend manager and have the backend loaded, which wasn't the case. I couldn't simply call _load_backend() in the get_backend_manager because this would lead to infinite recursion as _load_backend() also calls get_backend_manager. Therefore I had to refactor _load_backend to not call the former and directly fetch it through `aiida.backends.

Copy link
Member

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 clarification, very helpful!

One last question: _load_backend() now uses a backend manager that is then thrown away.
If these two things indeed have to go together, why not let _load_backend() also set self._backend_manager?

The current solution seems to duplicate the creation of the backend manager, once in _load_backend() and once in get_backend_manager().

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 left it in because I wasn't sure that there wasn't a code path (unlikely, but still) where the backend manager instance would not be constructed in _load_backend because it is in the conditional if configuration.BACKEND_UUID is None:. If that somehow is False the calling _load_backend will not create an instance of the backend manager. So calling get_backend_manager will return None. I have looked more into the code to see where the design mistake is, but I am not sure yet how to improve the flow. Most of this code that makes it complicated is to deal with the possibility of unloading/reloading different profiles, which is currently anyway not supported. Maybe it is best to leave like this for now, but if you prefer I can assign the backend manager in _load_backend to self._backend_manager and remove the construction from get_backend_manager.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, please add a comment in the corresponding issue so that we don't forget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

self._backend_manager = get_backend_manager(self.get_profile().database_backend)

return self._backend_manager
Expand Down
1 change: 1 addition & 0 deletions docs/source/reference/command_line.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ Below is a list with all available subcommands.
Commands:
integrity Check the integrity of the database and fix potential issues.
migrate Migrate the database to the latest schema version.
version Show the version of the database.


.. _reference:command-line:verdi-devel:
Expand Down
10 changes: 10 additions & 0 deletions tests/cmdline/commands/test_database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import enum

from click.testing import CliRunner
import pytest

from aiida.backends.testbase import AiidaTestCase
from aiida.cmdline.commands import cmd_database
Expand Down Expand Up @@ -170,3 +171,12 @@ def test_detect_invalid_nodes_unknown_node_type(self):
result = self.cli_runner.invoke(cmd_database.detect_invalid_nodes, [])
self.assertNotEqual(result.exit_code, 0)
self.assertIsNotNone(result.exception)


@pytest.mark.usefixtures('aiida_profile')
def tests_database_version(run_cli_command, manager):
"""Test the ``verdi database version`` command."""
backend_manager = manager.get_backend_manager()
result = run_cli_command(cmd_database.database_version)
assert result.output_lines[0].endswith(backend_manager.get_schema_generation_database())
assert result.output_lines[1].endswith(backend_manager.get_schema_version_database())