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

♻️ REFACTOR: Move archive backend to aiida/storage #5375

Merged
merged 26 commits into from
Mar 6, 2022

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Feb 22, 2022

This PR moves the archive storage backend implementation (sqlite_zip) into aiida/storage
alongside the "main" psql_dos backend.
It includes both the storage reading and migration functionality.

Inline with this synchronisation of the archive and the storage backend:

  • A number of the archive specific exceptions have been replaced with general storage ones:
    • UnreadableArchiveError/CorruptArchive -> CorruptStorage
    • IncompatibleArchiveVersionError -> IncompatibleStorageSchema
    • ArchiveMigrationError -> StorageMigrationError
    • ArchiveClosedError -> ClosedStorage
  • MIGRATE_LOGGER has been moved to aiida/storage/log.py
  • verdi archive inspect has been deprecated
    and replaced by verdi archive version and verdi archive info,
    to bring it inline with verdi storage.

alembic migrations have been added to the sqlite_zip migration logic,
to support future migrations of the sqlite database.
Alongside this, the non-null restrictions for legacy archive fields have been relaxed,
to improve robustness.
Subsequent migrations have been added;
first, to replace any existing (unwanted) null values with default values,
then to modify the schema to re-add the non-null constraints.

Finally, additional tests have been put in place (in tests/tools/archive/test_schema.py),
to ensure that the sqlite schema is always synchronised with the postgresql schema.


Notes

Writing of archive files is still specific to the archive functionality, due to the design constraint of an archive being contained within a single file.
Because of this, one cannot efficiently modify an existing archive, without

This should also eventually allow for the aiida/tools/archive/create.py::create_archive and aiida/tools/archive/imports.py::import_archive functions to be "re-conceptualised" as simply transfers of data between storage backends (see also aiidateam/AEP#31)

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Feb 27, 2022

Ok this is just about done, so ready for review.
I'll update the initial PR comment with some more description later

some potential TODOS:

@chrisjsewell chrisjsewell force-pushed the move-sqlite-zip branch 2 times, most recently from 6a06448 to bd1e3eb Compare February 27, 2022 10:26
@chrisjsewell chrisjsewell requested a review from sphuber February 27, 2022 10:27
@chrisjsewell chrisjsewell marked this pull request as ready for review February 27, 2022 10:27
@chrisjsewell chrisjsewell force-pushed the move-sqlite-zip branch 2 times, most recently from 0288589 to 5ee3590 Compare February 27, 2022 15:56
sphuber
sphuber previously approved these changes Mar 5, 2022
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @chrisjsewell . I have given it a bit of a look, but there is too much to really do it in detail and since this needs to go in for the beta release, I am approving. We should test ourselves the new archive functionality soon such that any bugs can be fixed a.s.a.p.

@chrisjsewell
Copy link
Member Author

We should test ourselves the new archive functionality soon such that any bugs can be fixed a.s.a.p.

Way ahead of you 😉 I already added #5407 on Friday

Just gotta update the PR description, then I'll merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants