Conversation
…oadd processing status sotrplib's new streaming coadd tool needs the same completed/failed/ permafail processing-status tracking that depth-1 maps already get, which requires letting time_domain_processing link to either a map or a coadd. While touching that table, also switch depth_one_maps.map_id and depth_one_coadds.coadd_id from integer to UUIDv7 primary keys (time-ordered, consistent with sotrplib's existing uuid7-standard usage), updating all 7 dependent FK columns to match. The new migration uses raw SQL table recreation for the column/constraint swap rather than batch_alter_table's create_primary_key()/ create_foreign_key(): in this SQLAlchemy/Alembic version those silently produced wrong results here (depth_one_maps ended up with no primary key at all; depth_one_sky_coverage's composite PK silently shrunk to just (x, y)), caused by columns individually declaring primary_key=True conflicting with the table-level constraint during batch reflection. Verified against real SQLite databases, including PK uniqueness enforcement and the CHECK constraint, before trusting it. Also fixes test_build_obslists, which asserted a relationship-list order that was never actually guaranteed by the DB -- it only held incidentally with sequential integer PKs -- and adds --coadd-id to the mapcatreset CLI. Migration is one-way: original integer IDs aren't recoverable once dropped, so downgrade() is intentionally not implemented. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…my's Uuid type The migration's raw-SQL backfill wrote new map_id/coadd_id values via str(uuid.uuid4()) (36-char, dashed), but SQLAlchemy's sa.Uuid() bind processor serializes UUIDs as 32-char undashed hex for SQLite. Since SQLite compares TEXT columns byte-for-byte, any query binding a fresh UUID value (e.g. an explicit map_id=/coadd_id= filter) silently matched zero rows against the migrated data. Fixes the backfill to write the undashed form, and adds an idempotent follow-up migration to normalize already-migrated databases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
merge the new main changes (fixed polynomial return type)
Pin the ruff dev dependency and CI action to 0.16.1 so lint results are reproducible, and apply the fixes 0.16.1 flags (import sorting, PEP 604 unions, quote style, line wrapping) so the uuid branch passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
axf295
left a comment
axf295
left a comment
There was a problem hiding this comment.
This PR is to use uuid7 as the map_id rather than a string rep of an integer.
I've also pinned the ruff version bc alembic migration formatting was failing.
| Revises: 46575bc0d660 | ||
| Create Date: 2026-07-16 13:15:00.000000 | ||
|
|
||
| This is a one-way migration: the original integer IDs are not recoverable |
There was a problem hiding this comment.
I think this is probably fine but it's a bit more of a Josh question. I would say it's presumably possible to do a downgrade. At the very least if we don't care about the ordering of our IDs then you could just go through the entries, strip them of their UUIDs, and give them a sequential integer ID. But again IDK if we need a downgrade here.
| once dropped, so downgrade() is intentionally not implemented (see | ||
| downgrade() below for details). | ||
|
|
||
| Step C (swapping in the new UUID columns and their constraints) is done |
There was a problem hiding this comment.
Yeah I ran into similar problems when I was working on this and while I never fully implemented a solution I think you do have to completely recreate the table (which is I think what is going on under the hood anyway).
| finalized. | ||
| """ | ||
|
|
||
| import uuid |
There was a problem hiding this comment.
I think this is a drop in replacement but I would do import uuid7 as uuid and include "uuid7-standard" as a requirement in pyproject.toml (will add a note there too). This is what is done for socat.
| new_column = f"{id_column}_new" | ||
| items = list(mapping.items()) | ||
| chunk_size = 500 | ||
| for start in range(0, len(items), chunk_size): |
There was a problem hiding this comment.
Not sure I get what the point of chunking is here if we're just going to loop through the chunk anyway?
| # Raw SQL only -- never import the (still-evolving) ORM model classes | ||
| # inside a migration. | ||
| map_rows = bind.execute(sa.text("SELECT map_id FROM depth_one_maps")).fetchall() | ||
| map_id_mapping = {row.map_id: uuid.uuid4() for row in map_rows} |
There was a problem hiding this comment.
Per my above comment I think we should use uuid7.
| """, | ||
| """ | ||
| INSERT INTO depth_one_maps_new | ||
| SELECT map_name, map_path, ivar_path, mean_time_path, tube_slot, |
There was a problem hiding this comment.
Just making sure I understand, we're selecting map_id_new from depth_one_map and inserting that into depth_one_map_new as map_id.
| bind.execute( | ||
| sa.text( | ||
| f"UPDATE {table} SET {column} = REPLACE({column}, '-', '') " | ||
| f"WHERE length({column}) = 36" |
There was a problem hiding this comment.
Is there any chance we get unlucky here and change some other column that happens to be length 36? Why not just check the column names?
|
|
||
| from sqlalchemy import Uuid | ||
| from sqlmodel import Field, Relationship, SQLModel | ||
| from uuid7 import create as uuid7_create |
There was a problem hiding this comment.
Wait so here you are using uuid7?
| __tablename__ = "depth_one_coadds" | ||
|
|
||
| coadd_id: int = Field(primary_key=True) | ||
| coadd_id: UUID = Field(default_factory=uuid7_create, primary_key=True, sa_type=Uuid) |
There was a problem hiding this comment.
socat uses import uuid7 as uuid, default_factory=uuid.create. Not sure it really matters but would be nice for these to be consistent.
There was a problem hiding this comment.
Just kidding saw the import statement.
| ivar_path=None, | ||
| frequency="f090", | ||
| ctime=ctime, | ||
| start_time=ctime - 500, |
There was a problem hiding this comment.
This is going to conflict with PR #35. Also now that I think about it it's gonna break your alembic revision, but that just involves changing the down revision labels. This is a PITA but I'd really like to merge 35 into main, and then pull those changes into this PR before merging. The opposite is probably more work.
2 participants
No description provided.