Manager: check if repository container has been initialised
#4889
Conversation
When loading the backend of the profile with the `Manager._load_backend` method, the `get_repository_container` method of the `Profile` is called, initialising a container for the profile in case is has not yet been initialised. This causes issues during the migration of the database/repository, since here the presence of the container directory results in a critical failure of the migration command. Here we add a check in the `_load_backend` method to see if the container of the profile has already been initialised. If it has, the repository uuid obtained from the container, if not it is set to `None`. This also removes the warning that was printed during the migration, since the repository uuid obtained from the database backend is also `None` in before the migration.
mbercx
added
priority/critical-blocking
|
I've tried this for my development database and it seems to work fine for the migration. I did get the following warning though: Warning:
Detected repository folders that were missing the required subfolder `path` or `raw_input`. The paths of those nodes repository folders have been written to a log file: /home/mbercx/envs/aiida-dev/setup/profile/migration-repository-missing-subfolder-4i32qdwj.jsonI do tend to mess around with my development database/repo quite a bit, so I may have messed it up somewhere. Will clone a different production database and try again with this branch. |
| container.init_container(clear=True, **self.defaults['repository']) # pylint: disable=unsubscriptable-object | ||
|
|
||
| return container | ||
|
|
||
| @property | ||
| def container_is_initialised(self): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @property | ||
| def _container_path(self): | ||
| """Return the path to the container of the profile file repository.""" | ||
| return os.path.join(self.repository_path, 'container') |
There was a problem hiding this comment.
self.repository_path should be a subclass of pathlib.Path.
| return os.path.join(self.repository_path, 'container') | |
| return self.repository_path / 'container' |
| return container.is_initialised | ||
|
|
||
| @property | ||
| def _container_path(self): |
There was a problem hiding this comment.
| def _container_path(self): | |
| def _container_path(self) -> pathlib.Path: |
| def container_is_initialised(self): | ||
| """Check if the container of the profile file repository has already been initialised.""" | ||
| from disk_objectstore import Container | ||
| filepath = self._container_path |
There was a problem hiding this comment.
Why didn't pre-commit catch this?
| filepath = self._container_path | |
| filepath = self._container_path |
| container.init_container(clear=True, **self.defaults['repository']) # pylint: disable=unsubscriptable-object | ||
|
|
||
| return container | ||
|
|
||
| @property | ||
| def container_is_initialised(self): |
There was a problem hiding this comment.
| def container_is_initialised(self): | |
| def container_is_initialised(self) -> bool: |
| container.init_container(clear=True, **self.defaults['repository']) # pylint: disable=unsubscriptable-object | ||
|
|
||
| return container | ||
|
|
||
| @property | ||
| def container_is_initialised(self): | ||
| """Check if the container of the profile file repository has already been initialised.""" |
There was a problem hiding this comment.
| """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.""" |
| filepath = self._container_path | ||
| container = Container(filepath) | ||
| return container.is_initialised |
There was a problem hiding this comment.
| filepath = self._container_path | |
| container = Container(filepath) | |
| return container.is_initialised | |
| return Container(self._container_path).is_initialised |
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## develop #4889 +/- ##
===========================================
- Coverage 80.08% 80.08% -0.00%
===========================================
Files 514 514
Lines 36567 36576 +9
===========================================
+ Hits 29280 29287 +7
- Misses 7287 7289 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Fixes #4885
When loading the backend of the profile with the
Manager._load_backendmethod, the
get_repository_containermethod of theProfileiscalled, initialising a container for the profile in case is has not yet
been initialised. This causes issues during the migration of the
database/repository, since here the presence of the container directory
results in a critical failure of the migration command.
Here we add a check in the
_load_backendmethod to see if thecontainer of the profile has already been initialised. If it has, the
repository uuid obtained from the container, if not it is set to
None.This also removes the warning that was printed during the migration
or the creation of a new profile, since the repository uuid obtained
from the database backend is also
Nonebefore the migration.