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

Manager: check if repository container has been initialised #4889

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Manager: check if repository container has been initialised #4889

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
17 changes: 15 additions & 2 deletions aiida/manage/configuration/profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,27 @@ def get_repository_container(self) -> 'Container':
"""
from disk_objectstore import Container

filepath = os.path.join(self.repository_path, 'container')
filepath = self._container_path
container = Container(filepath)

if not container.is_initialised:
if not self.container_is_initialised:
container.init_container(clear=True, **self.defaults['repository']) # pylint: disable=unsubscriptable-object

return container

@property
def container_is_initialised(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def container_is_initialised(self):
def container_is_initialised(self) -> bool:

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should remove container from these methods. I know that I called the method get_repository_container but the container part really comes from this particular implementation with the disk-objectstore. If we were to ever change it (don't think we will but in principle) the container concept may disappear. Maybe it would be better to have the following method and property names:

def get_repository() -> `Repository`
def is_repository_initialised
def initialise_repository

Looking at this more, currently get_repository_container returns the Container instance, but this is also a leaky abstraction. Some of the calls to this method currently just use it to get the UUID of the repo. Since then I added Profile.get_repository_uuid which is better and those places should use that instead.

Ok, this was a bit a train of thought and I am now thinking that we should just merge this and then I will do another PR with some proposed interface and name changes. What do you think?

Copy link
Contributor

@CasperWA CasperWA Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good to me. Then we can move on and you (@mbercx) can disregard my review comments since it should be updated anyway in the upcoming PR and they are not functionally critical.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, then I will merge this now and include your comments in the next PR. Then we can start testing the migration again. Thanks guys

"""Check if the container of the profile file repository has already been initialised."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Check if the container of the profile file repository has already been initialised."""
"""Return whether the container of the profile file repository has already been initialised."""

from disk_objectstore import Container
filepath = self._container_path
Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't pre-commit catch this?

Suggested change
filepath = self._container_path
filepath = self._container_path

container = Container(filepath)
return container.is_initialised
Comment on lines +149 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
filepath = self._container_path
container = Container(filepath)
return container.is_initialised
return Container(self._container_path).is_initialised

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if this is a more or less "constant" property, you could store it as a private variable and only create the Container instance once per call to this property, returning the "cached" private variable all the other times instead. I.e., you store the first result here in the private variable.


@property
def _container_path(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _container_path(self):
def _container_path(self) -> pathlib.Path:

"""Return the path to the container of the profile file repository."""
return os.path.join(self.repository_path, 'container')
Copy link
Contributor

Choose a reason for hiding this comment

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

self.repository_path should be a subclass of pathlib.Path.

Suggested change
return os.path.join(self.repository_path, 'container')
return self.repository_path / 'container'


@property
def uuid(self):
"""Return the profile uuid.
Expand Down
5 changes: 4 additions & 1 deletion aiida/manage/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ def _load_backend(self, schema_check: bool = True, repository_check: bool = True
# yet known, we issue a warning in the case the repo and database are incompatible. In the future this might
# then become an exception once we have verified that it is working reliably.
if repository_check and not profile.is_test_profile:
repository_uuid_config = profile.get_repository_container().container_id
if profile.container_is_initialised:
repository_uuid_config = profile.get_repository_container().container_id
else:
repository_uuid_config = None
repository_uuid_database = backend_manager.get_repository_uuid()

from aiida.cmdline.utils import echo
Expand Down