feat(BA-3209): Add artifact_storages common table#7057
feat(BA-3209): Add artifact_storages common table#7057jopemachine wants to merge 10 commits intomainfrom
artifact_storages common table#7057Conversation
artifact_storages common tableartifact_storages common table (WIP)
artifact_storages common table (WIP)artifact_storages common table
There was a problem hiding this comment.
Pull request overview
This PR introduces a new artifact_storages common table to centralize storage metadata management across object storage and VFS storage backends. The change refactors the storage architecture by extracting the name column from both object_storages and vfs_storages tables into a shared artifact_storages table.
Key changes:
- Added new
ArtifactStorageRowmodel to store common metadata (name, storage_id, type) for all storage backends - Migrated existing storage names to the new
artifact_storagestable via Alembic migration - Updated repository queries to use eager loading (
selectinload) for the newmetarelationship
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/models/artifact_storages.py |
Introduces new ArtifactStorageRow model with bidirectional relationships to object and VFS storage tables |
src/ai/backend/manager/models/alembic/versions/35dfab3b0662_add_artifact_storages_common_table.py |
Alembic migration that creates artifact_storages table and migrates existing data from both storage types |
src/ai/backend/manager/models/vfs_storage.py |
Removes name column, adds meta relationship, updates to_dataclass() to retrieve name from relationship |
src/ai/backend/manager/models/object_storage.py |
Removes name column, adds meta relationship, updates to_dataclass() to retrieve name from relationship |
src/ai/backend/manager/repositories/vfs_storage/db_source/db_source.py |
Adds eager loading of meta relationship in all query methods to prevent N+1 queries |
src/ai/backend/manager/repositories/object_storage/db_source/db_source.py |
Adds eager loading of meta relationship in all query methods to prevent N+1 queries |
src/ai/backend/manager/models/__init__.py |
Registers the new artifact_storages module in the models package |
changes/7057.feature.md |
Changelog entry describing the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...i/backend/manager/models/alembic/versions/35dfab3b0662_add_artifact_storages_common_table.py
Outdated
Show resolved
Hide resolved
...i/backend/manager/models/alembic/versions/35dfab3b0662_add_artifact_storages_common_table.py
Outdated
Show resolved
Hide resolved
9552aac to
4af738e
Compare
ecaaa5a to
bc37c21
Compare
55612c7 to
ab42a38
Compare
6a86174 to
4bedf71
Compare
| sa.Column("storage_id", GUID(), nullable=False), | ||
| sa.Column("type", sa.String(), nullable=False), | ||
| sa.PrimaryKeyConstraint("id", name=op.f("pk_artifact_storages")), | ||
| sa.UniqueConstraint("name", name=op.f("uq_artifact_storages_name")), |
There was a problem hiding this comment.
Is it acceptable to apply a global unique constraint to the artifact storage name? Could there be cases where the storage type differs but the names are identical? (As this is a policy matter, please verify this for me.)
There was a problem hiding this comment.
I think since artifact storage management is an admin-only feature, it doesn’t look awkward so far.
If, in the future, artifact storage management becomes available to regular users as well, it would make more sense to adjust the constraints at that point.
05278f8 to
46220f3
Compare
fregataa
left a comment
There was a problem hiding this comment.
I think that using SQLAlchemy Table inheritance would be great here.
src/ai/backend/manager/repositories/vfs_storage/db_source/db_source.py
Outdated
Show resolved
Hide resolved
12f09fb to
4200531
Compare
- Add artifact_storages table and migration script - Move name column to artifact_storages - Add proper selectinload in db_source - Add ID type and update version - Append admin prefix - Fix rename types and error domain - Add news fragment
Co-authored-by: octodog <mu001@lablup.com>
3e05451 to
5371c73
Compare
fregataa
left a comment
There was a problem hiding this comment.
Are there storage insert/update unit tests? It would be good to have them.
Plus, how do you think of Single Table Inheritance? Update and insert execution got complicated because of joined table inheritance, so I guess single table one makes it easy
| Uses plain UPDATE + SELECT instead of execute_updater's RETURNING+from_statement, | ||
| which is incompatible with JTI (RETURNING only includes child table columns). |
There was a problem hiding this comment.
is it not allowed to update VFSStorageRow?
I believe STI would not good for extensibility because it will introduce many nullable columns. |
resolves #7040 (BA-3209)
Checklist: (if applicable)
📚 Documentation preview 📚: https://sorna--7057.org.readthedocs.build/en/7057/
📚 Documentation preview 📚: https://sorna-ko--7057.org.readthedocs.build/ko/7057/