♻️ Reorganize so that storage table in hub can get an instance fk (#803)

* ♻️ Reorganize so that storage table in hub can get an instance fk * 💚 Fix * 💚 Fix
laminlabs · Jul 25, 2024 · 0e82b05 · 0e82b05
1 parent fe1a25d
commit 0e82b05
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 22 deletions.
diff --git a/lamindb_setup/_init_instance.py b/lamindb_setup/_init_instance.py
@@ -245,7 +245,10 @@ def init(
         if instance_state == "connected":
             settings.auto_connect = True  # we can also debate this switch here
             return None
-        ssettings = init_storage(storage, instance_id=instance_id)
+        # cannot past instance_id here because instance does not yet exist!
+        # the instance_id field of the storage table is populated at the end of
+        # init_instance_hub
+        ssettings = init_storage(storage)
         isettings = InstanceSettings(
             id=instance_id,  # type: ignore
             owner=settings.user.handle,

diff --git a/lamindb_setup/core/_hub_core.py b/lamindb_setup/core/_hub_core.py
@@ -93,20 +93,21 @@ def _select_storage(
         return False
     else:
         existing_storage = response.data[0]
-        if ssettings._instance_id is not None:
-            # consider storage settings that are meant to be managed by an instance
-            if UUID(existing_storage["instance_id"]) != ssettings._instance_id:
-                # everything is alright if the instance_id matches
-                # we're probably just switching storage locations
-                # below can be turned into a warning and then delegate the error
-                # to a unique constraint violation below
-                raise ValueError(
-                    f"Storage root {root} is already managed by instance {existing_storage['instance_id']}."
-                )
-        else:
-            # if the request is agnostic of the instance, that's alright,
-            # we'll update the instance_id with what's stored in the hub
-            ssettings._instance_id = UUID(existing_storage["instance_id"])
+        if existing_storage["instance_id"] is not None:
+            if ssettings._instance_id is not None:
+                # consider storage settings that are meant to be managed by an instance
+                if UUID(existing_storage["instance_id"]) != ssettings._instance_id:
+                    # everything is alright if the instance_id matches
+                    # we're probably just switching storage locations
+                    # below can be turned into a warning and then delegate the error
+                    # to a unique constraint violation below
+                    raise ValueError(
+                        f"Storage root {root} is already managed by instance {existing_storage['instance_id']}."
+                    )
+            else:
+                # if the request is agnostic of the instance, that's alright,
+                # we'll update the instance_id with what's stored in the hub
+                ssettings._instance_id = UUID(existing_storage["instance_id"])
         ssettings._uuid_ = UUID(existing_storage["id"])
         if update_uid:
             ssettings._uid = existing_storage["lnid"]
@@ -145,19 +146,17 @@ def _init_storage(ssettings: StorageSettings, client: Client) -> None:
         id = uuid.uuid5(uuid.NAMESPACE_URL, root)
     else:
         id = uuid.uuid4()
-    if ssettings._instance_id is None:
-        logger.warning(
-            f"will manage storage location {ssettings.root_as_str} with instance {settings.instance.slug}"
-        )
-        ssettings._instance_id = settings.instance._id
+    instance_id_hex = (
+        None if ssettings._instance_id is None else ssettings._instance_id.hex
+    )
     fields = {
         "id": id.hex,
         "lnid": ssettings.uid,
         "created_by": settings.user._uuid.hex,  # type: ignore
         "root": root,
         "region": ssettings.region,
         "type": ssettings.type,
-        "instance_id": ssettings._instance_id.hex,
+        "instance_id": instance_id_hex,
         # the empty string is important as we want the user flow to be through LaminHub
         # if this errors with unique constraint error, the user has to update
         # the description in LaminHub

diff --git a/tests/hub-local/test_all.py b/tests/hub-local/test_all.py
@@ -118,8 +118,9 @@ def create_myinstance(create_testadmin1_session):  # -> Dict
         id=instance_id,
         owner=usettings.handle,
         name="myinstance",
+        # cannot yet pass instance_id here as it does not yet exist
         storage=init_storage_base(
-            "s3://lamindb-ci/myinstance", instance_id=instance_id
+            "s3://lamindb-ci/myinstance",
         ),
         db=db_str,
     )