-
Notifications
You must be signed in to change notification settings - Fork 229
Fix sqlite_zip
profile deletion with missing aiida archive
#6929
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
Fix sqlite_zip
profile deletion with missing aiida archive
#6929
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6929 +/- ##
==========================================
+ Coverage 79.03% 79.03% +0.01%
==========================================
Files 566 566
Lines 43657 43666 +9
==========================================
+ Hits 34500 34508 +8
- Misses 9157 9158 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
768a0ef
to
26d6d7f
Compare
Ping @agoscinski @khsrali @unkcpz if anyone of you has the time and willingness to review 🫶 |
07bbf0f
to
b67087f
Compare
dbc3408
to
73a6180
Compare
73a6180
to
6cc1c73
Compare
Would you have time to review this during the coding days, @agoscinski? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think we should spend time on fixing errors that only happen if the user messes with files in the .aiida
folder. It is on purpose a hidden folder. AFAIK we have no explicit documentation to mess with it (if we have we should consolidate on this, IMO user should not mess with it). Otherwise you can fix infinitely many problems, since you cannot make any assumptions. I would rather safe the testing time and more important introduced complexity than fixing this issue and concentrate on the underlying reason of why one would delete the .aiida
folder. Should we just add a simple nuke aiida CLI that deletes everything properly?
tests/conftest.py
Outdated
@@ -383,13 +383,13 @@ def empty_config(tmp_path) -> Config: | |||
|
|||
|
|||
@pytest.fixture | |||
def profile_factory() -> Profile: | |||
def profile_factory(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still the type you are returning can be specifieyCallable[Concatenate[str, P], Profile]
using P = ParamSpec("P")
I don't know how much it makes sense to have type hints in tests, but it seems also like utils we would like to ship for developer at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably removed this because mypy
otherwise complains with Untyped decorator makes function "profile_factory" untyped
in my IDE (and the type annotation was wrong anyway). To properly fix this, one would have to type it also where it's being used. I think it's OK to just ignore the mypy error. I'd also be fine to just remove the type hints in the tests, but as it was there previously, I keep it for now. Lastly, I'm not sure if the tests/conftest
fixtures are ones that we will ship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right because of the decorator it does not work, sorry forgot
Ah, I think there was a misunderstanding. With
This should be a valid pathway and just delete the profile, instead of excepting. |
sqlite_zip
profile deletion with non-existent .aiida
filesqlite_zip
profile deletion with missing aiida archive
6cc1c73
to
de788ff
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
2e2d947
to
df5eb35
Compare
Closes #6928.
With an
sqlite_zip
profile created from an aiida archive file, when the file is deleted, and afterwards the profile, including all data, it would lead to anUnreachableStorage
exception with the following traceback:Traceback
Warning: Deleting profile `archive`, including all data. Traceback (most recent call last): File "/home/geiger_j/.aiida_venvs/aiida-dev/bin/verdi", line 8, in <module> sys.exit(verdi()) File "/home/geiger_j/.aiida_venvs/aiida-dev/lib/python3.10/site-packages/click/core.py", line 1157, in __call__ return self.main(*args, **kwargs) File "/home/geiger_j/.aiida_venvs/aiida-dev/lib/python3.10/site-packages/click/core.py", line 1078, in main rv = self.invoke(ctx) File "/home/geiger_j/.aiida_venvs/aiida-dev/lib/python3.10/site-packages/click/core.py", line 1688, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/home/geiger_j/.aiida_venvs/aiida-dev/lib/python3.10/site-packages/click/core.py", line 1688, in invoke return _process_result(sub_ctx.command.invoke(sub_ctx)) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/cmdline/groups/verdi.py", line 119, in invoke return ctx.invoke(self.callback, **ctx.params) File "/home/geiger_j/.aiida_venvs/aiida-dev/lib/python3.10/site-packages/click/core.py", line 783, in invoke return __callback(*args, **kwargs) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/cmdline/commands/cmd_profile.py", line 287, in profile_delete get_config().delete_profile(profile.name, delete_storage=delete_data) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/manage/configuration/config.py", line 585, in delete_profile storage = storage_cls(profile) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/storage/sqlite_zip/backend.py", line 190, in __init__ validate_storage(self._path) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/storage/sqlite_zip/migrator.py", line 61, in validate_storage schema_version_archive = read_version(inpath) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/storage/sqlite_zip/utils.py", line 141, in read_version metadata = extract_metadata(path, search_limit=search_limit) File "/home/geiger_j/aiida_projects/aiida-dev/git-repos/aiida-core/src/aiida/storage/sqlite_zip/utils.py", line 103, in extract_metadata raise UnreachableStorage(f'path not found: {path}') aiida.common.exceptions.UnreachableStorage: path not found: /home/geiger_j/aiida_projects/aiida-dev/process.aiida
A check is now added for the storage backend and existence of the
.aiida
file, and, forsqlite_zip
and a non-existent.aiida
file, extra logging is issued anddelete_storage
set toFalse
in the call todelete_profile
. This is done rather than just wrapping the call in a try-except, to be more explicit and not accidentally captureUnreachableStorage
exceptions that might be due to other causes, as the exception actually occurs when trying to read the metadata of the backend, not when the file is actually being deleted.