🐛 Prepare for deletion of the storage_id column from the instance table #915
Conversation
lamindb_setup/core/_hub_core.py
Outdated
| @@ -251,7 +254,13 @@ def _delete_instance( | |||
| check_storage_is_empty( | |||
| root_path, account_for_sqlite_file=account_for_sqlite_file | |||
| ) | |||
| _update_instance_record(instance_with_storage["id"], {"storage_id": None}, client) | |||
| # remove this code block after migrtion, i.e. deleting 'storage_id' column of 'instance' | |||
There was a problem hiding this comment.
We can also remove it straight away if we just set the values to None in the hub. I'd actually prefer that.
There was a problem hiding this comment.
I would prefer to keep it until we actually migrate.
There was a problem hiding this comment.
Why? I don't see the reason.
It makes the code complicated and unreadable. There is a really nice potential for a refactor once we remove this because the whole backward forward coupling of instance and storage is then finally disentangled.
There was a problem hiding this comment.
Because we're no longer populating this column the tests are already passing even without deleting anything on the hub.
We still have to go through with it though so that deletions of legacy instances don't start failing.
There was a problem hiding this comment.
Made an issue for this: https://github.com/laminlabs/laminhub/issues/1748
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #915 +/- ##
==========================================
+ Coverage 83.77% 83.79% +0.01%
==========================================
Files 43 43
Lines 3434 3432 -2
==========================================
- Hits 2877 2876 -1
+ Misses 557 556 -1 ☔ View full report in Codecov by Sentry. |
| client.table("instance") | ||
| .select("*, storage!instance_storage_id_87963cc8_fk_storage_id(*)") |
There was a problem hiding this comment.
Oh no, was this somehow forgotten before?
There was a problem hiding this comment.
This FK should have been replaced with the storage_instance_id FK 9 months ago when it was replaced everywhere else. 🤔
There was a problem hiding this comment.
Ok, this is only used during deletion; so it's not a big deal.
lamindb-setup/lamindb_setup/core/_hub_core.py
Line 217 in db43bf3
Maybe it was re-introduced at a later point or something similar.
No user will hit it.
There was a problem hiding this comment.
Yes, it was forgotten, but it is pretty specific thing, only called on deletion by id, so not really user facing.
Always search default storage through the storage table.