Merge pull request #53 from lsst-dm/tickets/DM-53459

DM-53459: Fix Alembic/SQLAlchemy warnings.

Author: andy-slac

migrations/_oneshot/datasets/int_1.0.0_to_uuid_1.0.0/2101fbf51ad3.py

@@ -3,7 +3,6 @@
 Revision ID: 2101fbf51ad3
 Revises: 635083325f20
 Create Date: 2021-05-04 16:31:05.926989
-
 """
 from __future__ import annotations
 
@@ -452,24 +451,24 @@ def _drop_columns(table_name: str, table_info: TableInfo, schema: str | None) ->
         for index_dict in table_info.indices:
             index_name = index_dict["name"]
             _LOG.debug("Dropping index %s", index_dict)
-            batch_op.drop_index(index_name)  # type: ignore[attr-defined]
+            batch_op.drop_index(index_name)
 
         for unique_dict in table_info.unique_constraints:
             unique_name = unique_dict["name"]
             _LOG.debug("Dropping unique constraint %s", unique_name)
-            batch_op.drop_constraint(unique_name)  # type: ignore[attr-defined]
+            batch_op.drop_constraint(unique_name)
 
         for fk_dict in table_info.foreign_keys:
             fk_name = fk_dict["name"]
             _LOG.debug("Dropping foreign key %s", fk_name)
-            batch_op.drop_constraint(fk_name)  # type: ignore[attr-defined]
+            batch_op.drop_constraint(fk_name)
 
         if table_info.primary_key:
             _LOG.debug("Primary key: %s", table_info.primary_key)
             pk_name = table_info.primary_key["name"]
             if pk_name:
                 _LOG.debug("Dropping primary key %s", pk_name)
-                batch_op.drop_constraint(pk_name)  # type: ignore[attr-defined]
+                batch_op.drop_constraint(pk_name)
             else:
                 dialect = op.get_bind().dialect
                 if dialect.name == "sqlite":
@@ -483,13 +482,13 @@ def _drop_columns(table_name: str, table_info: TableInfo, schema: str | None) ->
                         idx = columns.index(id_col)
                         columns[idx] += "_uuid"
                         _LOG.debug("Creating primary key %s", columns)
-                        batch_op.create_primary_key(  # type: ignore[attr-defined]
+                        batch_op.create_primary_key(
                             f"{table_name}_pkey", columns
                         )
 
         # drop column
         _LOG.debug("Dropping column %s", id_col)
-        batch_op.drop_column(id_col)  # type: ignore[attr-defined]
+        batch_op.drop_column(id_col)
 
     # drop a sequence as well
     if table_name == "dataset":
@@ -509,7 +508,7 @@ def _rename_column(table_name: str, schema: str | None) -> None:
     _LOG.debug("Renaming uuid column in table %s to %s", table_name, id_col)
 
     with op.batch_alter_table(table_name, schema) as batch_op:
-        batch_op.alter_column(  # type: ignore[attr-defined]
+        batch_op.alter_column(
             f"{id_col}_uuid", new_column_name=id_col, nullable=False
         )
 
@@ -524,20 +523,20 @@ def _make_indices(table_name: str, table_info: TableInfo, schema: str | None) ->
                 pk_name = f"{table_name}_pkey"
             _LOG.debug("Adding primary key %s", pk_name)
             columns = table_info.primary_key["constrained_columns"]
-            batch_op.create_primary_key(pk_name, columns)  # type: ignore[attr-defined]
+            batch_op.create_primary_key(pk_name, columns)
 
         for unique_dict in table_info.unique_constraints:
             unique_name = unique_dict["name"]
             _LOG.debug("Adding unique constraint %s", unique_name)
             columns = unique_dict["column_names"]
-            batch_op.create_unique_constraint(unique_name, columns)  # type: ignore[attr-defined]
+            batch_op.create_unique_constraint(unique_name, columns)
 
         for index_dict in table_info.indices:
             index_name = index_dict["name"]
             _LOG.debug("Adding index %s", index_dict)
             columns = index_dict["column_names"]
             unique = index_dict["unique"]
-            batch_op.create_index(index_name, columns, unique=unique)  # type: ignore[attr-defined]
+            batch_op.create_index(index_name, columns, unique=unique)
 
         for fk_dict in table_info.foreign_keys:
             fk_name = fk_dict["name"]
@@ -551,7 +550,7 @@ def _make_indices(table_name: str, table_info: TableInfo, schema: str | None) ->
             ondelete = None
             if table_name.startswith(DYNAMIC_TABLES_PREFIX):
                 ondelete = "CASCADE"
-            batch_op.create_foreign_key(  # type: ignore[attr-defined]
+            batch_op.create_foreign_key(
                 fk_name, ref_table, local_cols, remote_cols, ondelete=ondelete, referent_schema=ref_schema
             )
 

migrations/datasets/4e2d7a28475b.py

@@ -3,7 +3,6 @@
 Revision ID: 4e2d7a28475b
 Revises: 2101fbf51ad3
 Create Date: 2023-03-31 22:25:32.651155
-
 """
 
 import datetime
@@ -149,10 +148,10 @@ def _migrate_default(
     with op.batch_alter_table("dataset", schema) as batch_op:
         if server_default is None:
             # If removing a default it has to be done first
-            batch_op.alter_column("ingest_date", server_default=None)  # type: ignore[attr-defined]
+            batch_op.alter_column("ingest_date", server_default=None)
     with op.batch_alter_table("dataset", schema) as batch_op:
         # Change the type of the column.
-        batch_op.alter_column(  # type: ignore[attr-defined]
+        batch_op.alter_column(
             "ingest_date", type_=column_type, server_default=server_default
         )
 

migrations/dimensions-config/bf6308af80aa.py

@@ -3,7 +3,6 @@
 Revision ID: bf6308af80aa
 Revises: 380002bcbb26
 Create Date: 2022-05-16 12:11:17.906600
-
 """
 import logging
 
@@ -74,7 +73,7 @@ def _FKConstraint(
     schema: str | None = None,
     ondelete: str | None = None,
 ) -> sa.schema.ForeignKeyConstraint:
-    """Create foreign key constraint."""
+    # Create foreign key constraint.
     fk_name = "_".join(["fkey", src_table, tgt_table] + tgt_columns + src_columns)
     fk_name = shrinkDatabaseEntityName(fk_name, bind)
     tgt_columns = [f"{tgt_table}.{col}" for col in tgt_columns]
@@ -85,7 +84,8 @@ def _FKConstraint(
 
 def _migrate_visit_definition() -> None:
     """Make new visit_system_membership table, fill from existing data
-    in visit_definition."""
+    in visit_definition.
+    """
 
     mig_context = context.get_context()
     bind = mig_context.bind
@@ -147,23 +147,38 @@ def _migrate_visit_definition() -> None:
         visit_definition.columns["visit"],
     ).distinct()
     sql = visit_membership.insert().from_select(["instrument", "visit_system", "visit"], selection)
-    op.execute(sql)  # type: ignore[arg-type]
+    op.execute(sql)
 
     # Drop visit_system from visit_definition.
     with op.batch_alter_table("visit_definition", schema=schema) as batch_op:
         # visit_system is in PK. Postgres drops PK when the column is dropped,
         # but sqlite complains that column cannot be dropped if it's in PK.
-        # The workaround is to create PK with new columns, but Postgres also
-        # requires existing PK to be dropped first, and in sqlite we do not
-        # even have name for PK constraint, so it cannot be dropped
-        # explicitly.
+        # Dropping PK in SQLite cannot be done as we generate PKs without name.
+        # Simply trying to replace PK using different columns generates
+        # SAWarning. The workaround is to re-create PK with old columns and
+        # name it, then drop that named PK. Postgres also requires existing PK
+        # to be dropped first before re-creating it.
+
+        # Drop PK in Postgres.
         if bind.dialect.name == "postgresql":
-            batch_op.drop_constraint("visit_definition_pkey")  # type: ignore[attr-defined]
-        batch_op.create_primary_key(  # type: ignore[attr-defined]
+            batch_op.drop_constraint("visit_definition_pkey")
+
+        # Re-create PK and give it a name.
+        batch_op.create_primary_key(
+            "visit_definition_pkey", ["instrument", "visit_system", "exposure"]
+        )
+
+        # Now drop named PK.
+        batch_op.drop_constraint("visit_definition_pkey")
+
+        # Create PK with different columns.
+        batch_op.create_primary_key(
             "visit_definition_pkey", ["instrument", "exposure", "visit"]
         )
-        batch_op.drop_index("visit_definition_fkidx_instrument_visit_system")  # type: ignore[attr-defined]
-        batch_op.drop_column("visit_system")  # type: ignore[attr-defined]
+
+        # Finally drop index and column.
+        batch_op.drop_index("visit_definition_fkidx_instrument_visit_system")
+        batch_op.drop_column("visit_system")
 
 
 def _migrate_instrument() -> None:
@@ -202,10 +217,10 @@ def _migrate_visit() -> None:
 
     _LOG.info("migrating visit table")
     with op.batch_alter_table("visit", schema=schema) as batch_op:
-        batch_op.drop_index("visit_fkidx_instrument_visit_system")  # type: ignore[attr-defined]
-        batch_op.drop_column("visit_system")  # type: ignore[attr-defined]
-        batch_op.add_column(sa.Column("seq_num", sa.BigInteger))  # type: ignore[attr-defined]
-        batch_op.add_column(sa.Column("azimuth", sa.Float))  # type: ignore[attr-defined]
+        batch_op.drop_index("visit_fkidx_instrument_visit_system")
+        batch_op.drop_column("visit_system")
+        batch_op.add_column(sa.Column("seq_num", sa.BigInteger))
+        batch_op.add_column(sa.Column("azimuth", sa.Float))
 
     # Fill seq_num column with the lowest value of matching exposure.seq_num.
     #
@@ -259,10 +274,10 @@ def _migrate_exposure(has_simulated: bool) -> None:
 
     _LOG.info("migrating exposure table")
     with op.batch_alter_table("exposure", schema=schema) as batch_op:
-        batch_op.add_column(sa.Column("seq_start", sa.BigInteger))  # type: ignore[attr-defined]
-        batch_op.add_column(sa.Column("seq_end", sa.BigInteger))  # type: ignore[attr-defined]
-        batch_op.add_column(sa.Column("azimuth", sa.Float))  # type: ignore[attr-defined]
-        batch_op.add_column(sa.Column("has_simulated", sa.Boolean))  # type: ignore[attr-defined]
+        batch_op.add_column(sa.Column("seq_start", sa.BigInteger))
+        batch_op.add_column(sa.Column("seq_end", sa.BigInteger))
+        batch_op.add_column(sa.Column("azimuth", sa.Float))
+        batch_op.add_column(sa.Column("has_simulated", sa.Boolean))
 
     table = sa.schema.Table("exposure", metadata, autoload_with=bind, schema=schema)
     op.execute(

migrations/dimensions-config/c5ae3a2cd7c2.py

@@ -3,7 +3,6 @@
 Revision ID: c5ae3a2cd7c2
 Revises: bf6308af80aa
 Create Date: 2022-11-25 12:04:18.424257
-
 """
 
 import sqlalchemy as sa
@@ -76,7 +75,7 @@ def _update_config(config: dict) -> dict:
                 column_type = sa.Text()
             else:
                 column_type = sa.String(column_size)
-            batch_op.alter_column(column, type_=column_type)  # type: ignore[attr-defined]
+            batch_op.alter_column(column, type_=column_type)
 
             assert mig_context.bind is not None, "Requires an existing bind"
             if mig_context.bind.dialect.name == "sqlite":
@@ -85,6 +84,6 @@ def _update_config(config: dict) -> dict:
                 constraint = f'length("{column}")<={column_size} AND length("{column}")>=1'
                 if old_column_size <= 32:
                     # Constraint only exists for shorter strings.
-                    batch_op.drop_constraint(constraint_name)  # type: ignore[attr-defined]
+                    batch_op.drop_constraint(constraint_name)
                 if column_size <= 32:
-                    batch_op.create_check_constraint(constraint_name, sa.text(constraint))  # type: ignore
+                    batch_op.create_check_constraint(constraint_name, sa.text(constraint))

migrations/dimensions/035dcf13ef18.py

@@ -3,7 +3,6 @@
 Revision ID: 035dcf13ef18
 Revises: 1601d5973bf8
 Create Date: 2022-05-19 15:40:18.561744
-
 """
 import logging
 from typing import cast
@@ -26,9 +25,11 @@
 
 
 def upgrade() -> None:
-    """Summary of changes for this migration:
+    """Upgrade from version 6.0.1 to version 6.0.2.
+
+    Summary of changes for this migration:
 
-        - implied dimension columns (non-PK foreign keys) add NOT NULL
+        - implied dimension columns (non-PK foreign keys) add NOT NULL.
 
     Note that schema digests are not updated. Just before we updated code for
     6.0.2 versions we disabled validation of checksums. Older code that uses
@@ -82,7 +83,7 @@ def _do_migration(nullable: bool, manager_version: str) -> None:
                         _LOG.info("Add NULL to column %s.%s", element_name, other_element)
                     else:
                         _LOG.info("Add NOT NULL to column %s.%s", element_name, other_element)
-                    batch_op.alter_column(other_element, nullable=nullable)  # type: ignore[attr-defined]
+                    batch_op.alter_column(other_element, nullable=nullable)
 
     attributes.update_manager_version(MANAGER, manager_version)
 

mypy.ini

@@ -5,6 +5,9 @@ warn_redundant_casts = True
 ignore_missing_imports = False
 disallow_untyped_defs = True
 disallow_incomplete_defs = True
+strict_equality = True
+warn_unused_ignores = True
+warn_unreachable = True
 
 [mypy-sqlalchemy.*]
 ignore_missing_imports = True
@@ -18,21 +21,3 @@ ignore_missing_imports = True
 [mypy-lsst.*]
 ignore_missing_imports = True
 ignore_errors = True
-
-# Check all of daf.butler_migrate...
-
-[mypy-lsst.daf.butler_migrate.*]
-ignore_missing_imports = False
-ignore_errors = False
-disallow_untyped_defs = True
-disallow_incomplete_defs = True
-strict_equality = True
-warn_unreachable = True
-warn_unused_ignores = True
-
-; [mypy-lsst.daf.butler.*]
-; ignore_missing_imports = False
-; ignore_errors = False
-
-; [mypy-lsst.daf.butler.cli.*]
-; ignore_missing_imports = True

pyproject.toml

@@ -151,5 +151,5 @@ checks = [
 exclude = [
     "^test_.*",  # Do not test docstrings in test code.
     '^commands\.',  # Click docstrings, not numpydoc
-    '\.__exit__',  # Do not check docstrings in context methods.
+    '\._',  # Do not check docstrings in private methods.
 ]

python/lsst/daf/butler_migrate/config.py

@@ -74,6 +74,8 @@ def from_mig_path(
         cfg.set_main_option("script_location", alembic_folder)
         cfg.set_section_option("alembic", "file_template", "%%(rev)s")
         cfg.set_section_option("alembic", "prepend_sys_path", ".")
+        # We use space as separateor in version_locations below.
+        cfg.set_section_option("alembic", "path_separator", "space")
         _LOG.debug(
             "alembic_folder: %r, single_tree: %r, one_shot_tree: %r",
             alembic_folder,