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

AiiDA storage abstraction improvements #5353

Open
4 of 11 tasks
chrisjsewell opened this issue Feb 16, 2022 · 2 comments
Open
4 of 11 tasks

AiiDA storage abstraction improvements #5353

chrisjsewell opened this issue Feb 16, 2022 · 2 comments

Comments

@chrisjsewell
Copy link
Member

chrisjsewell commented Feb 16, 2022

Some TODOs left over from #5330:

  • Make Profile.storage_cls select the backend class via an entry_point (Done in Make storage backends pluginnable and dynamically loaded through entry points #5501)
  • In Profile.set_storage and Profile.set_process_controller, use a classmethod on the backend class, to validate the config
  • Profile.repository_path should be moved onto the backend class (Done in Deprecate Profile.repository_path #5516)
    • A potential issue here though, is that repository_path is now also used for sandbox folders, which is really a different concept
  • Profile.rmq_prefix and Profile.get_rmq_url should be moved to a separate "backend class for process control", similar to Profile.storage_cls
  • Currently, Config.delete_profile contains logic very specific to the psql_dos, and so needs to be refactored (Done in verdi profile delete: Make it storage plugin agnostic #6224)
    • By extension, this would also require changes in verdi profile delete
    • This is just about the only code left, hard-coded to a backend (except verdi setup I guess)
  • In verdi setup there is no check that the database/repository URL you supply is initially empty, e.g. is not being used by another profile
    • I don't believe this has ever been the case
    • I guess it would be a very rare corner-case, hence not a priority
  • aiida/backends/sqlalchemy/alembic_cli.py::alembic_revision currently fails, because alembic.RevisionContext._run_environment has issues with multiple heads (it works if you comment out the initial auto-generate check in that method)
  • Improve ArchiveReadOnlyBackend, in light of the backend abstract improvements
    • Eventually we should be able to load a Profile with e.g. the storage.backend = "archive" and the config pointing to the archive path.
  • "Unify" tests/backends/aiida_sqlalchemy/test_utils.py and tests/backends/aiida_sqlalchemy/migrations/conftest.py
  • aiida/restapi/common/utils.py::close_thread_connection should eventually not be hard-coded to the psql_dos backend
    • More generally, there should be a consistent mechanism for using aiida in multi-threaded mode, rather than relying on thread-local sqla sessions
    • Now that most global variables have been consolidated onto Manager, we could perhaps make this thread-local
  • profile_context should probably restore any originally loaded profile, on exit

Originally posted by @chrisjsewell in #5330 (comment)

@ltalirz
Copy link
Member

ltalirz commented Feb 24, 2022

Just mentioning that there are many Github actions test setups in plugins that set the AIIDA_TEST_BACKEND variable, either to django or to sqlalchemy.
That django no longer works makes sense, but do we also need to raise when people use sqlalchemy?

Perhaps we can continue allowing that? At the very least, the error message ValueError: Unknown backend 'sqlalchemy' read from AIIDA_TEST_BACKEND environment variable should say what the allowed backends are.
psql_dos is not very self-explanatory (even I don't know what dos stands for)

Edit: ok, I get it now

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Feb 24, 2022

even I don't know what dos stands for

psql_dos=postgresql+disk-objectstore, i.e. what the storage format actually is, not what python package happens to be used

I would just update the error message, to tell people to change to psql_dos.
Presumably, people using AIIDA_TEST_BACKEND are doing it to test against both django and sqlachemy, so it's in their interest to know that they don't need to do this anymore, and they can basically drop the use of AIIDA_TEST_BACKEND

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

No branches or pull requests

2 participants