`StorageBackend`: Add the `initialise` method #5760

sphuber · 2022-11-15T11:16:21Z

The current interface of the StorageBackend provides two method to
control the initialisation and lifetime of a storage:

migrate
_clear

The migrate method will initialise a completely new storage or migrate
an already initialised backend to the latest schema version, whereas the
_clear method will clear all data from the storage. In reality, the
current implementation of _clear will actually completely destroy the
container and recreate it from scratch, which causes a new repository
UUID to be generated.

Conceptually, but also practically, it is beneficial to restructure this
design slightly. When using storage for a testing profile, it may be
necessary to completely reset a storage backend and reinitialise it.

Therefore the initialise method is added to the StorageBackend
abstract base class. This method replaces the migrate method as the
method to be called when a new storage backend is created and needs to
be initialised. It should make sure that at the end of the initialisation
the storage is at the latest schema version, i.e., by calling migrate
internally.

The method takes an argument reset, which is False by default, but
when set to True, the storage backend should be completely reset before
reinitialising. Typically this entails that the storage is completely
destroyed and reinitialised in a fresh state.

This is used by the TemporaryProfileManager test utility class when
a new temporary profile is created. Although the tests in aiida-core
essentially always use the PsqlDosBackend and so the Postgres database
will always have the correct initialisation, storage backends are now
pluginable and in plugins we may want to switch between storage backends
within a test suite, in which case the storage backends needs to be fully
reinitialised.

sphuber · 2022-11-15T11:19:06Z

@ltalirz this is an alternative to #5758 . By adding the initialise method with the option to completely reset the storage, I no longer need to remove the autouse=True from the aiida_profile fixture. This allows me in the plugin to call profile.storage_cls.initialise(reset=True) if I need to reuse the database for a different storage backend implementation. I haven't added the new fixtures on top of this PR yet (which are part of #5758 ) but I have tested them locally. I would suggest we review and merge this PR first, and then I will open a PR that adds the new public fixtures that replace the aiida.manage.tests.main module.

sphuber · 2022-11-15T11:19:21Z

Pinging @chrisjsewell in case you are interested to comment

ltalirz

thanks a lot @sphuber , I think these changes make the codebase clearer.

just some minor questions/comments

aiida/orm/implementation/storage_backend.py

aiida/storage/sqlite_zip/backend.py

ltalirz · 2022-11-15T12:58:24Z

aiida/storage/sqlite_temp/backend.py

@@ -70,6 +70,10 @@ def version_head(cls) -> str:
    def version_profile(cls, profile: Profile) -> str | None:  # pylint: disable=unused-argument
        return get_schema_version_head()

+    @classmethod
+    def initialise(cls, profile: 'Profile', reset: bool = False) -> None:
+        pass


Just for me to understand: why is initialisation and migration a no-op for this storage backend?
Should it not also raise a NotImplementedError?

To be honest, I don't know. I just replicated the behavior for the migrate method. These backends were added by @chrisjsewell and are mostly used for export archives, which are initialised manually maybe in the export code. That being said, I think this means this will fail when users try to initialise a profile of this storage type through verdi setup. But then again, that was already the case. This PR is not changing any of that. Might want to open an issue to see whether this should be possible.

ltalirz · 2022-11-15T13:02:31Z

aiida/storage/psql_dos/migrator.py

+        with self._connection_context() as connection:
+            if inspect(connection).has_table(self.alembic_version_tbl_name):
+                for table_name in (
+                    'db_dbgroup_dbnodes', 'db_dbgroup', 'db_dblink', 'db_dbnode', 'db_dblog', 'db_dbauthinfo',


just checking whether this list of tables can either be deduced from somewhere (or, if not, whether it should be stored in a reusable constant)

Not sure to be honest. There is one similar list in the PsqlDosBackend._clear method minus the db_dbsetting table. I tried to change as little as possible and just get the minimum that is required.

yeh it should likely be deduced by reflection

Done. Also changed the backend to call the implementation of the migrator, since that was also hard-coding the tables in the _clear method.

I have isolated the changes that I think are responsible. Just pushed the commit without those changes to see if the test times return to normal. Then I will add the isolated changes in a separate commit and run again. That should give us a good sense whether the change in time is real. Also easier to review the changes and spot a mistake in connection handling.

Oh yeh, I remember there being issues with reflection and opening/closing sessions

Ok, without the table delete changes, test times are normal (~13-14 mins): https://github.com/sphuber/aiida-core/actions/runs/3479079616/jobs/5817199424

Oh yeah, I remember there being issues with reflection and opening/closing sessions

The initial change I added of using reflection caused the tests to hang. I tracked this down to reflection indeed opening a session and so the _clear call of the next test would hang, since there was an outstanding connection. I could fix this by explicitly closing the session in _clear. However, I think there may still be a problem with the handling of connections in PsqlDosMigrator. Many methods need a connection and they will just open one through the _connection_context method. However, it doesn't seem that these ever get closed. The opened connection is simply returned and not yielded and then closed at the end of the context manager. I will push a commit to add explicit closing and see if this fixes the "too many active connections" problem. It will probably not fix the increase in time though.

Ok, when adding the delete_tables method the tests for Python 3.8 run in reasonably normal time, with tests for Python 3.10 being slightly longer at ~15-16 minutes. Before I saw ~20 minutes so think there is quite some variability between runs independent of changes.

However, the tests do fail due to too many connections being opened (only for Python 3.8 though, but this could also be random potentially). Will now push a tentative fix for this and see if that resolves things.

aiida/storage/psql_dos/migrator.py

chrisjsewell · 2022-11-15T14:17:06Z

aiida/storage/sqlite_zip/backend.py

@@ -78,6 +78,10 @@ def create_profile(path: str | Path, options: dict | None = None) -> Profile:
    def version_profile(cls, profile: Profile) -> Optional[str]:
        return read_version(profile.storage_config['path'], search_limit=None)

+    @classmethod
+    def initialise(cls, profile: 'Profile', reset: bool = False) -> None:


This should be implemented in this PR

Not sure why, given that this backend is read-only and so it cannot be used for creating a normal profile through verdi setup nor used as a test profile, which are the only two places where initialise is called. But I implemented it regardless.

aiida/orm/implementation/storage_backend.py

aiida/storage/psql_dos/migrator.py

ltalirz

All relevant points from my side have been addressed (as far as I understand the sqlite_temp backend initialisation is indeed no change in behavior, and so it does not need to be fixed here, although it would be good to address eventually).

sphuber · 2022-11-16T08:52:02Z

@chrisjsewell all comments addressed. Please give it a review

sphuber · 2022-11-16T16:02:47Z

So it seems the refactoring of the reflection on the schema to remove the hardcoding of the table names needs some more work. But this hard-coding was already present in the code and is not something that is changed by this PR, I propose to move this to a separate PR in order to unblock this one.

sphuber · 2022-11-16T16:10:29Z

I have opened #5769 to track the problem and will move the last two commits I had here with the partial implementation to a separate PR where we can work on working out the final kinks.

ltalirz · 2022-11-16T16:17:03Z

Sounds good to me!

chrisjsewell

Please trigger the nightly tests. Since this contains all the migration tests, which may well be affected by this PR

sphuber · 2022-11-17T10:53:20Z

Please trigger the nightly tests. Since this contains all the migration tests, which may well be affected by this PR

Done, and they pass

The current interface of the `StorageBackend` provides two method to control the initialisation and lifetime of a storage: * `migrate` * `_clear` The `migrate` method will initialise a completely new storage or migrate an already initialised backend to the latest schema version, whereas the `_clear` method will clear all data from the storage. In reality, the current implementation of `_clear` will actually completely destroy the container and recreate it from scratch, which causes a new repository UUID to be generated. Conceptually, but also practically, it is beneficial to restructure this design slightly. When using storage for a testing profile, it may be necessary to completely reset a storage backend and reinitialise it. Therefore the `initialise` method is added to the `StorageBackend` abstract base class. This method replaces the `migrate` method as the method to be called when a new storage backend is created and needs to be initialised. It should make sure that at the end of the initialisation the storage is at the latest schema version, i.e., by calling `migrate` internally. The method takes an argument `reset`, which is `False` by default, but when set to `True`, the storage backend should be completely reset before reinitialising. Typically this entails that the storage is completely destroyed and reinitialised in a fresh state. This is used by the `TemporaryProfileManager` test utility class when a new temporary profile is created. Although the tests in `aiida-core` essentially always use the `PsqlDosBackend` and so the Postgres database will always have the correct initialisation, storage backends are now pluginable and in plugins we may want to switch between storage backends within a test suite, in which case the storage backends needs to be fully reinitialised.

This allows the workflow to be triggered manually. This is useful if a PR is created that doesn't trigger the normal conditions, for example the files marked in the `pull_request` trigger are not modified, but we still want to verify that the changes pass the tests.

sphuber requested a review from ltalirz November 15, 2022 11:16

ltalirz reviewed Nov 15, 2022

View reviewed changes

chrisjsewell reviewed Nov 15, 2022

View reviewed changes

aiida/storage/psql_dos/migrator.py Outdated Show resolved Hide resolved

chrisjsewell reviewed Nov 15, 2022

View reviewed changes

aiida/storage/psql_dos/migrator.py Outdated Show resolved Hide resolved

chrisjsewell reviewed Nov 15, 2022

View reviewed changes

aiida/orm/implementation/storage_backend.py Outdated Show resolved Hide resolved

sphuber force-pushed the feature/storage-backend-initialise branch from 8586378 to a3e994f Compare November 15, 2022 15:07

ltalirz reviewed Nov 15, 2022

View reviewed changes

aiida/storage/psql_dos/migrator.py Outdated Show resolved Hide resolved

ltalirz reviewed Nov 15, 2022

View reviewed changes

aiida/storage/psql_dos/migrator.py Outdated Show resolved Hide resolved

ltalirz self-requested a review November 15, 2022 15:21

ltalirz approved these changes Nov 15, 2022

View reviewed changes

sphuber force-pushed the feature/storage-backend-initialise branch from a3e994f to 637f359 Compare November 15, 2022 15:30

sphuber requested a review from chrisjsewell November 16, 2022 08:51

sphuber force-pushed the feature/storage-backend-initialise branch from 637f359 to fe57fd4 Compare November 16, 2022 08:51

sphuber force-pushed the feature/storage-backend-initialise branch 3 times, most recently from 6f3830c to e4d4dba Compare November 16, 2022 11:52

sphuber force-pushed the feature/storage-backend-initialise branch from 8226e70 to 77185fa Compare November 16, 2022 16:07

sphuber force-pushed the feature/storage-backend-initialise branch from 77185fa to ff667a6 Compare November 17, 2022 07:57

chrisjsewell suggested changes Nov 17, 2022

View reviewed changes

sphuber requested a review from chrisjsewell November 17, 2022 10:53

sphuber added 2 commits November 17, 2022 17:41

sphuber force-pushed the feature/storage-backend-initialise branch from 319a337 to 198e9d2 Compare November 17, 2022 16:41

sphuber merged commit 38d40d5 into aiidateam:main Nov 17, 2022

sphuber deleted the feature/storage-backend-initialise branch November 17, 2022 17:03

sphuber mentioned this pull request Nov 17, 2022

Fixtures: Modularize fixtures creating AiiDA test instance and profile #5758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`StorageBackend`: Add the `initialise` method #5760

`StorageBackend`: Add the `initialise` method #5760

sphuber commented Nov 15, 2022

sphuber commented Nov 15, 2022

sphuber commented Nov 15, 2022

ltalirz left a comment

ltalirz Nov 15, 2022

sphuber Nov 15, 2022

ltalirz Nov 15, 2022

sphuber Nov 15, 2022

chrisjsewell Nov 15, 2022

sphuber Nov 15, 2022

ltalirz Nov 15, 2022

sphuber Nov 16, 2022

chrisjsewell Nov 16, 2022

sphuber Nov 16, 2022

sphuber Nov 16, 2022

sphuber Nov 16, 2022

chrisjsewell Nov 15, 2022

sphuber Nov 16, 2022

ltalirz left a comment •

edited

Loading

sphuber commented Nov 16, 2022

sphuber commented Nov 16, 2022

sphuber commented Nov 16, 2022

ltalirz commented Nov 16, 2022

chrisjsewell left a comment

sphuber commented Nov 17, 2022

StorageBackend: Add the initialise method #5760

StorageBackend: Add the initialise method #5760

Conversation

sphuber commented Nov 15, 2022

sphuber commented Nov 15, 2022

sphuber commented Nov 15, 2022

ltalirz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz left a comment • edited Loading

Choose a reason for hiding this comment

sphuber commented Nov 16, 2022

sphuber commented Nov 16, 2022

sphuber commented Nov 16, 2022

ltalirz commented Nov 16, 2022

chrisjsewell left a comment

Choose a reason for hiding this comment

sphuber commented Nov 17, 2022

`StorageBackend`: Add the `initialise` method #5760

`StorageBackend`: Add the `initialise` method #5760

ltalirz left a comment •

edited

Loading