From 2e0b86b01ed47403a7cbd6de42f41be3ad6876f3 Mon Sep 17 00:00:00 2001 From: Koncopd Date: Wed, 20 May 2026 14:26:15 +0300 Subject: [PATCH 1/5] refactor storage delete --- lamindb/models/storage.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/lamindb/models/storage.py b/lamindb/models/storage.py index 143f047ab..d73480178 100644 --- a/lamindb/models/storage.py +++ b/lamindb/models/storage.py @@ -11,7 +11,6 @@ from lamindb_setup import settings as setup_settings from lamindb_setup.core._hub_core import ( delete_storage_record, - get_storage_records_for_instance, select_space, update_storage_with_space, ) @@ -351,8 +350,6 @@ def delete(self, permanent: bool | None = None) -> None: # type: ignore Args: permanent: `False` raises an error, as soft delete is impossible. """ - from .. import settings - if permanent is False: raise ValueError( "Soft delete is not possible for Storage, " @@ -366,22 +363,21 @@ def delete(self, permanent: bool | None = None) -> None: # type: ignore super(SQLRecord, self).delete() return None # now the complicated case of a written/managed storage location - check_storage_is_empty(self.path) - assert settings.storage.root_as_str != self.root, ( # noqa: S101 + root_path = self.path + check_storage_is_empty(root_path) + assert setup_settings.storage.root_as_str != self.root, ( # noqa: S101 "Cannot delete the current storage location, switch to another." ) - if setup_settings.user.handle != "anonymous": # only attempt if authenticated - storage_records = get_storage_records_for_instance( - # only query those storage records on the hub that are managed by the current instance - setup_settings.instance._id + ssettings = StorageSettings(root_path) + if ( + setup_settings.user.handle != "anonymous" + and (storage_record := ssettings.hub_record) is not None + ): + assert storage_record["is_default"] in {False, None}, ( # noqa: S101 + "Cannot delete default storage of instance." ) - for storage_record in storage_records: - if storage_record["lnid"] == self.uid: - assert storage_record["is_default"] in {False, None}, ( # noqa: S101 - "Cannot delete default storage of instance." - ) - delete_storage_record(storage_record) - ssettings = StorageSettings(self.root) + delete_storage_record(storage_record) + if ssettings._mark_storage_root.exists(): ssettings._mark_storage_root.unlink( missing_ok=True # this is totally weird, but needed on Py3.11 From 1e3330093d04d232ce1a7ba55fb6c1e0bbaa770d Mon Sep 17 00:00:00 2001 From: Koncopd Date: Wed, 20 May 2026 14:51:33 +0300 Subject: [PATCH 2/5] refactor and allow skipping empty check --- lamindb/models/storage.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lamindb/models/storage.py b/lamindb/models/storage.py index d73480178..3afa793db 100644 --- a/lamindb/models/storage.py +++ b/lamindb/models/storage.py @@ -339,16 +339,15 @@ def save(self, *args, **kwargs): super().save(*args, **kwargs) return self - def delete(self, permanent: bool | None = None) -> None: # type: ignore - # type ignore is there because we don't use a trash here unlike everywhere else + def delete(self, permanent: bool | None = None, enforce_empty: bool = True) -> None: # type: ignore + # type ignore is there because we don't have kwargs as in the superclass """Delete the storage location. - This errors in case the storage location is not empty. - Unlike other `SQLRecord`-based registries, this does *not* move the storage record into the trash. Args: permanent: `False` raises an error, as soft delete is impossible. + enforce_empty: `True` raises an error if the storage location is not empty. """ if permanent is False: raise ValueError( @@ -363,11 +362,10 @@ def delete(self, permanent: bool | None = None) -> None: # type: ignore super(SQLRecord, self).delete() return None # now the complicated case of a written/managed storage location - root_path = self.path - check_storage_is_empty(root_path) assert setup_settings.storage.root_as_str != self.root, ( # noqa: S101 "Cannot delete the current storage location, switch to another." ) + root_path = self.path ssettings = StorageSettings(root_path) if ( setup_settings.user.handle != "anonymous" @@ -376,10 +374,10 @@ def delete(self, permanent: bool | None = None) -> None: # type: ignore assert storage_record["is_default"] in {False, None}, ( # noqa: S101 "Cannot delete default storage of instance." ) + check_storage_is_empty(root_path, raise_error=enforce_empty) + if ssettings._mark_storage_root.exists(): + ssettings._mark_storage_root.unlink( + missing_ok=True # this is totally weird, but needed on Py3.11 + ) delete_storage_record(storage_record) - - if ssettings._mark_storage_root.exists(): - ssettings._mark_storage_root.unlink( - missing_ok=True # this is totally weird, but needed on Py3.11 - ) super(SQLRecord, self).delete() From a46d1287e931b19191d99382c716bc7b6b175e75 Mon Sep 17 00:00:00 2001 From: Koncopd Date: Wed, 20 May 2026 15:06:35 +0300 Subject: [PATCH 3/5] fix --- tests/storage/test_storage_lifecycle.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/storage/test_storage_lifecycle.py b/tests/storage/test_storage_lifecycle.py index 95e5fc2d1..246230075 100644 --- a/tests/storage/test_storage_lifecycle.py +++ b/tests/storage/test_storage_lifecycle.py @@ -56,16 +56,7 @@ def test_switch_delete_storage_location(): assert "Cannot delete storage with artifacts in current instance." in err.exconly() artifact.delete(permanent=True, storage=False) - # still some files in there - with pytest.raises(ln.setup.errors.StorageNotEmpty) as err: - new_storage.delete() - assert ( - "'s3://lamindb-ci/test-settings-switch-storage/.lamindb' contains 1 objects" - in err.exconly() - ) - # now delete the artifact so that the storage location is empty - artifact.path.unlink() with pytest.raises(AssertionError) as err: new_storage.delete() assert ( @@ -73,13 +64,22 @@ def test_switch_delete_storage_location(): in err.exconly() ) - # check all attempts unsuccessful so far - assert check_storage_location_on_hub_exists(new_storage.uid) - # switch back to default storage ln.settings.storage = "./default_storage_unit_storage" + + # still some files in there + with pytest.raises(ln.setup.errors.StorageNotEmpty) as err: + new_storage.delete() + assert ( + "'s3://lamindb-ci/test-settings-switch-storage/.lamindb' contains 1 objects" + in err.exconly() + ) storage_marker = ln.UPath(new_storage_location) / ".lamindb/storage_uid.txt" assert storage_marker.exists() + assert check_storage_location_on_hub_exists(new_storage.uid) + + # now delete the artifact so that the storage location is empty + artifact.path.unlink() new_storage.delete() assert not check_storage_location_on_hub_exists(new_storage.uid) assert not storage_marker.exists() From 2474b2b66c84daf9ac0dc50b6dadc3b761c71ebb Mon Sep 17 00:00:00 2001 From: Koncopd Date: Wed, 20 May 2026 15:42:59 +0300 Subject: [PATCH 4/5] fix --- lamindb/models/storage.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/lamindb/models/storage.py b/lamindb/models/storage.py index 3afa793db..088848e46 100644 --- a/lamindb/models/storage.py +++ b/lamindb/models/storage.py @@ -362,11 +362,11 @@ def delete(self, permanent: bool | None = None, enforce_empty: bool = True) -> N super(SQLRecord, self).delete() return None # now the complicated case of a written/managed storage location - assert setup_settings.storage.root_as_str != self.root, ( # noqa: S101 + root = self.root + assert setup_settings.storage.root_as_str != root, ( # noqa: S101 "Cannot delete the current storage location, switch to another." ) - root_path = self.path - ssettings = StorageSettings(root_path) + ssettings = StorageSettings(root) if ( setup_settings.user.handle != "anonymous" and (storage_record := ssettings.hub_record) is not None @@ -374,10 +374,18 @@ def delete(self, permanent: bool | None = None, enforce_empty: bool = True) -> N assert storage_record["is_default"] in {False, None}, ( # noqa: S101 "Cannot delete default storage of instance." ) - check_storage_is_empty(root_path, raise_error=enforce_empty) - if ssettings._mark_storage_root.exists(): - ssettings._mark_storage_root.unlink( - missing_ok=True # this is totally weird, but needed on Py3.11 - ) + # cleanup storage before deleting the storage record + # in case credentials refresh is required + _check_cleanup_storage(ssettings, enforce_empty) delete_storage_record(storage_record) + else: + _check_cleanup_storage(ssettings, enforce_empty) super(SQLRecord, self).delete() + + +def _check_cleanup_storage(ssettings: StorageSettings, enforce_empty: bool = True): + check_storage_is_empty(ssettings.root, raise_error=enforce_empty) + if ssettings._mark_storage_root.exists(): + ssettings._mark_storage_root.unlink( + missing_ok=True # this is totally weird, but needed on Py3.11 + ) From 00aa1e19a0d98c7457020791fecdd9c18c8b9db5 Mon Sep 17 00:00:00 2001 From: Koncopd Date: Wed, 27 May 2026 11:51:20 +0300 Subject: [PATCH 5/5] eject --- lamindb/models/storage.py | 38 +++++++++++++++++++------ tests/storage/test_storage_lifecycle.py | 7 +++-- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/lamindb/models/storage.py b/lamindb/models/storage.py index 088848e46..633a446e1 100644 --- a/lamindb/models/storage.py +++ b/lamindb/models/storage.py @@ -354,13 +354,33 @@ def delete(self, permanent: bool | None = None, enforce_empty: bool = True) -> N "Soft delete is not possible for Storage, " "use 'permanent=True' or 'permanent=None' for permanent deletion." ) - assert not self.artifacts.exists(), ( - "Cannot delete storage with artifacts in current instance." - ) # noqa: S101 - # the simple case of a read-only storage location - if self.instance_uid != setup_settings.instance.uid: - super(SQLRecord, self).delete() - return None + + is_managed_by_current_instance = ( + self.instance_uid == setup_settings.instance.uid + ) + is_empty_storage = not self.artifacts.exists() + if is_empty_storage: + if not is_managed_by_current_instance: + # the simple case of a read-only storage location without artifacts + super(SQLRecord, self).delete() + return None + else: + if enforce_empty: + raise ValueError( + "Cannot delete storage with artifacts when 'enforce_empty=True'." + ) + if is_managed_by_current_instance: + logger.important( + "cannot delete managed storage because it has artifacts, making it non-managed" + ) + self.instance_uid = None + self.save() + else: + logger.important( + "cannot delete non-managed storage because it has artifacts" + ) + return None + # now the complicated case of a written/managed storage location root = self.root assert setup_settings.storage.root_as_str != root, ( # noqa: S101 @@ -380,7 +400,9 @@ def delete(self, permanent: bool | None = None, enforce_empty: bool = True) -> N delete_storage_record(storage_record) else: _check_cleanup_storage(ssettings, enforce_empty) - super(SQLRecord, self).delete() + + if is_empty_storage: + super(SQLRecord, self).delete() def _check_cleanup_storage(ssettings: StorageSettings, enforce_empty: bool = True): diff --git a/tests/storage/test_storage_lifecycle.py b/tests/storage/test_storage_lifecycle.py index 246230075..58d315c48 100644 --- a/tests/storage/test_storage_lifecycle.py +++ b/tests/storage/test_storage_lifecycle.py @@ -51,9 +51,12 @@ def test_switch_delete_storage_location(): assert new_storage.root in artifact.path.as_posix() # artifacts exist - with pytest.raises(AssertionError) as err: + with pytest.raises(ValueError) as err: new_storage.delete() - assert "Cannot delete storage with artifacts in current instance." in err.exconly() + assert ( + "Cannot delete storage with artifacts when 'enforce_empty=True'." + in err.exconly() + ) artifact.delete(permanent=True, storage=False)