Merge pull request #170 from lsst-dm/tickets/DM-48808/typing_fixes

DM-48808 : Typing Fixes

Author: tcjennings

src/lsst/cmservice/common/panda.py

@@ -71,7 +71,7 @@ def refresh_panda_token(url: str, data: dict[str, str]) -> str | None:
     os.environ["PANDA_AUTH_ID_TOKEN"] = config.panda.id_token
     # - update token expiry
     decoded_token = decode_id_token(config.panda.id_token)
-    config.panda.token_expiry = float(decoded_token["exp"])  # type: ignore
+    config.panda.token_expiry = float(decoded_token["exp"])  # type: ignore[assignment]
     if TYPE_CHECKING:
         # the validation machinery of the pyantic field handles conversion
         # from float to datetime.
@@ -126,7 +126,7 @@ def get_panda_token() -> str | None:
     try:
         if config.panda.token_expiry is None:
             decoded_token = decode_id_token(config.panda.id_token)
-            config.panda.token_expiry = float(decoded_token["exp"])  # type: ignore
+            config.panda.token_expiry = float(decoded_token["exp"])  # type: ignore[assignment]
         if TYPE_CHECKING:
             # the validation machinery of the pyantic field handles conversion
             # from float to datetime.

src/lsst/cmservice/db/row.py

@@ -83,15 +83,24 @@ async def get_rows(
         parent_class = kwargs.get("parent_class")
 
         q = select(cls)
-        # FIXME: Being a mixin leads to loose typing here.
-        # Is there a better way?
         if hasattr(cls, "parent_id"):
+            # FIXME All of these tests assert that parent_class is not None
+            #       otherwise it would raise an AttributeError; the constraint
+            #       might as well be satisfied by the first id matching without
+            #       also evaluating additional options. If the gimmick is that
+            #       the method can be invoked with one of parent_id, _name, or
+            #       the parent class, it is invalidated by the fact that
+            #       parent_class is used in all three cases, so defining any of
+            #       the others adds no value.
+            if TYPE_CHECKING:
+                assert parent_class is not None
             if parent_class is not None:
-                q = q.where(parent_class.id == cls.parent_id)  # type: ignore
+                parent_id_ = getattr(cls, "parent_id")
+                q = q.where(parent_class.id == parent_id_)
             if parent_name is not None:
-                q = q.where(parent_class.fullname == parent_name)  # type: ignore
+                q = q.where(parent_class.fullname == parent_name)
             if parent_id is not None:
-                q = q.where(parent_class.id == parent_id)  # type: ignore
+                q = q.where(parent_class.id == parent_id)
         q = q.offset(skip).limit(limit)
         results = await session.scalars(q)
         return results.all()

src/lsst/cmservice/handlers/jobs.py

@@ -68,6 +68,8 @@ async def _write_script(
         parent: ElementMixin,
         **kwargs: Any,
     ) -> StatusEnum:
+        if TYPE_CHECKING:
+            assert isinstance(parent, Job)
         # Database operations
         await session.refresh(parent, attribute_names=["c_"])
         data_dict = await script.data_dict(session)
@@ -90,7 +92,7 @@ async def _write_script(
         # yaml template, NOT the yaml template itself!
         workflow_config: dict[str, Any] = {}
         workflow_config["project"] = "DEFAULT"
-        workflow_config["campaign"] = parent.c_.name  # type: ignore
+        workflow_config["campaign"] = parent.c_.name
         workflow_config["pipeline_yaml"] = pipeline_yaml
         workflow_config["lsst_version"] = lsst_version
         workflow_config["lsst_distrib_dir"] = lsst_distrib_dir
@@ -170,7 +172,7 @@ async def _write_script(
             in_collection = input_colls
 
         payload = {
-            "name": parent.c_.name,  # type: ignore
+            "name": parent.c_.name,
             "butler_config": butler_repo,
             "output_run_collection": run_coll,
             "input_collection": in_collection,
@@ -216,7 +218,9 @@ async def _check_slurm_job(
         if fake_status is not None:
             wms_job_id = "fake_job"
         else:  # pragma: no cover
-            bps_dict = await parse_bps_stdout(script.log_url)  # type: ignore
+            if TYPE_CHECKING:
+                assert script.log_url is not None
+            bps_dict = await parse_bps_stdout(script.log_url)
             wms_job_id = self.get_job_id(bps_dict)
         await parent.update_values(session, wms_job_id=wms_job_id)
         return slurm_status
@@ -243,7 +247,9 @@ async def _check_htcondor_job(
             if fake_status is not None:
                 wms_job_id = "fake_job"
             else:  # pragma: no cover
-                bps_dict = await parse_bps_stdout(script.log_url)  # type: ignore
+                if TYPE_CHECKING:
+                    assert script.log_url is not None
+                bps_dict = await parse_bps_stdout(script.log_url)
                 wms_job_id = self.get_job_id(bps_dict)
             await parent.update_values(session, wms_job_id=wms_job_id)
         return htcondor_status

src/lsst/cmservice/handlers/scripts.py

@@ -2,7 +2,7 @@
 
 import os
 import textwrap
-from typing import Any
+from typing import TYPE_CHECKING, Any
 
 from anyio import Path
 from sqlalchemy.ext.asyncio import async_scoped_session
@@ -470,6 +470,8 @@ async def _write_script(
         **kwargs: Any,
     ) -> StatusEnum:
         test_type_and_raise(parent, Step, "PrepareStepScriptHandler._write_script parent")
+        if TYPE_CHECKING:
+            assert isinstance(parent, Step)
 
         resolved_cols = await script.resolve_collections(session)
         data_dict = await script.data_dict(session)
@@ -482,7 +484,7 @@ async def _write_script(
 
         prereq_colls: list[str] = []
 
-        all_prereqs = await parent.get_all_prereqs(session)  # type: ignore
+        all_prereqs = await parent.get_all_prereqs(session)
         for prereq_step in all_prereqs:
             prereq_step_colls = await prereq_step.resolve_collections(session)
             prereq_colls.append(prereq_step_colls["step_public_output"])

src/lsst/cmservice/web_app/pages/campaigns.py

@@ -1,4 +1,5 @@
 from collections.abc import Sequence
+from typing import TYPE_CHECKING
 
 from sqlalchemy import select
 from sqlalchemy.ext.asyncio import async_scoped_session
@@ -9,6 +10,8 @@
 
 
 async def get_campaign_details(session: async_scoped_session, campaign: Campaign) -> dict:
+    if TYPE_CHECKING:
+        assert isinstance(campaign.data, dict)
     collections = await campaign.resolve_collections(session, throw_overrides=False)
     groups = await get_campaign_groups(session, campaign)
     no_groups_completed = len([group for group in groups if group.status == StatusEnum.accepted])
@@ -26,7 +29,7 @@ async def get_campaign_details(session: async_scoped_session, campaign: Campaign
         "name": campaign.name,
         "production_name": campaign.fullname.split("/")[0],
         "fullname": campaign.fullname,
-        "lsst_version": campaign.data["lsst_version"],  # type: ignore
+        "lsst_version": campaign.data["lsst_version"],
         "source": collections.get("campaign_source", ""),
         "status": map_status(campaign.status),
         "groups_completed": f"{no_groups_completed} of {len(groups)} groups completed",

tests/cli/test_others.py

@@ -22,21 +22,21 @@ async def test_others_cli(uvicorn: UvicornProcess, api_version: str) -> None:
     assert len(check) == 0
 
     result = runner.invoke(client_top, "product_set list --output yaml")
-    check = check_and_parse_result(result, list[models.ProductSet])  # type: ignore
-    assert len(check) == 0
+    products = check_and_parse_result(result, list[models.ProductSet])
+    assert len(products) == 0
 
     result = runner.invoke(client_top, "script_dependency list --output yaml")
-    check = check_and_parse_result(result, list[models.Dependency])  # type: ignore
-    assert len(check) == 0
+    dependencies = check_and_parse_result(result, list[models.Dependency])
+    assert len(dependencies) == 0
 
     result = runner.invoke(client_top, "script_error list --output yaml")
-    check = check_and_parse_result(result, list[models.ScriptError])  # type: ignore
-    assert len(check) == 0
+    scripts = check_and_parse_result(result, list[models.ScriptError])
+    assert len(scripts) == 0
 
     result = runner.invoke(client_top, "task_set list --output yaml")
-    check = check_and_parse_result(result, list[models.TaskSet])  # type: ignore
-    assert len(check) == 0
+    tasks = check_and_parse_result(result, list[models.TaskSet])
+    assert len(tasks) == 0
 
     result = runner.invoke(client_top, "wms_task_report list --output yaml")
-    check = check_and_parse_result(result, list[models.WmsTaskReport])  # type: ignore
-    assert len(check) == 0
+    reports = check_and_parse_result(result, list[models.WmsTaskReport])
+    assert len(reports) == 0

tests/cli/util_functions.py

@@ -10,6 +10,7 @@
 from lsst.cmservice.common.enums import LevelEnum, StatusEnum
 
 T = TypeVar("T")
+E = TypeVar("E", models.Group, models.Campaign, models.Step, models.Job)
 
 
 def check_and_parse_result(
@@ -412,15 +413,15 @@ def check_scripts(
 def check_get_methods(
     runner: CliRunner,
     client_top: BaseCommand,
-    entry: models.ElementMixin,
+    entry: E,
     entry_class_name: str,
     entry_class: TypeAlias = models.ElementMixin,
 ) -> None:
     result = runner.invoke(client_top, f"{entry_class_name} get all --output yaml --row_id {entry.id}")
     check_get = check_and_parse_result(result, entry_class)
 
     assert check_get.id == entry.id, "pulled row should be identical"
-    assert check_get.level == entry.level, "pulled row db_id should be identical"  # type: ignore
+    assert check_get.level == entry.level, "pulled row db_id should be identical"
 
     result = runner.invoke(client_top, f"{entry_class_name} get by_name --output yaml --name {entry.name}")
     check_get = check_and_parse_result(result, entry_class)

tests/common/test_config.py

@@ -74,7 +74,7 @@ def test_config_datetime() -> None:
         assert config.panda.token_expiry is None
 
     # test validation and coercion on assignment
-    config.panda.token_expiry = 1740147265  # type: ignore
+    config.panda.token_expiry = 1740147265  # type: ignore[assignment]
     assert isinstance(config.panda.token_expiry, datetime)
     assert config.panda.token_expiry.tzinfo is UTC
 

tests/db/test_group.py

@@ -66,11 +66,11 @@ async def test_group_db(engine: AsyncEngine) -> None:
             await db.Group.delete_row(session, -99)
 
         # run group specific method tests
-        check = await entry.get_campaign(session)
-        assert check.name == f"camp0_{uuid_int}", "should return same name as camp0"
+        campaign = await entry.get_campaign(session)
+        assert campaign.name == f"camp0_{uuid_int}", "should return same name as camp0"
 
-        check = await entry.children(session)  # type: ignore
-        assert len(list(check)) == 1, "length of children should be 1"  # type: ignore
+        children = await entry.children(session)
+        assert len(list(children)) == 1, "length of children should be 1"
 
         # check update methods
         await check_update_methods(session, entry, db.Group)

tests/db/test_job.py

@@ -1,6 +1,7 @@
 import importlib
 import os
 import uuid
+from typing import TYPE_CHECKING
 
 import pytest
 import structlog
@@ -62,6 +63,8 @@ async def test_job_db(engine: AsyncEngine) -> None:
         entry = check_getall[0]  # defining single unit for later
 
         parent = await entry.get_parent(session)
+        if TYPE_CHECKING:
+            assert isinstance(parent, db.Group)
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.running)
         sleep_time = await parent.estimate_sleep_time(session)
@@ -77,11 +80,11 @@ async def test_job_db(engine: AsyncEngine) -> None:
         campaign = await entry.get_campaign(session)
         assert campaign.name == f"camp0_{uuid_int}", "should return same name as camp0"
 
-        check = await entry.get_siblings(session)
-        assert len(list(check)) == 0, "length of siblings should be 0"
+        siblings = await entry.get_siblings(session)
+        assert len(list(siblings)) == 0, "length of siblings should be 0"
 
-        check = await entry.get_errors(session)  # type: ignore
-        assert len(check) == 0, "length of errors should be 0"
+        errors_ = await entry.get_errors(session)
+        assert len(errors_) == 0, "length of errors should be 0"
 
         sleep_time = await campaign.estimate_sleep_time(session)
         assert sleep_time == 10, "Wrong sleep time"
@@ -94,42 +97,42 @@ async def test_job_db(engine: AsyncEngine) -> None:
 
         # check on the rescue job
         with pytest.raises(errors.CMTooFewAcceptedJobsError):
-            await parent.rescue_job(session)  # type: ignore
+            await parent.rescue_job(session)
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.rescuable)
-        job2 = await parent.rescue_job(session)  # type: ignore
+        job2 = await parent.rescue_job(session)
 
         with pytest.raises(errors.CMBadStateTransitionError):
-            await parent.mark_job_rescued(session)  # type: ignore
+            await parent.mark_job_rescued(session)
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.rescuable)
         with pytest.raises(errors.CMBadStateTransitionError):
-            await parent.mark_job_rescued(session)  # type: ignore
+            await parent.mark_job_rescued(session)
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.rescuable)
         await db.Job.update_row(session, job2.id, status=StatusEnum.accepted)
 
-        rescued = await parent.mark_job_rescued(session)  # type: ignore
+        rescued = await parent.mark_job_rescued(session)
         assert len(rescued) == 1, "Wrong number of rescued jobs"
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.accepted)
         await db.Job.update_row(session, job2.id, status=StatusEnum.accepted)
         with pytest.raises(errors.CMTooManyActiveScriptsError):
-            await parent.mark_job_rescued(session)  # type: ignore
+            await parent.mark_job_rescued(session)
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.rescuable)
         await db.Job.update_row(session, job2.id, status=StatusEnum.rescuable)
 
-        job3 = await parent.rescue_job(session)  # type: ignore
+        job3 = await parent.rescue_job(session)
 
         await db.Job.update_row(session, entry.id, status=StatusEnum.rescued)
         await db.Job.update_row(session, job2.id, status=StatusEnum.failed, superseded=True)
         await db.Job.update_row(session, job3.id, status=StatusEnum.rescuable)
 
         with pytest.raises(errors.CMTooFewAcceptedJobsError):
-            await parent.mark_job_rescued(session)  # type: ignore
+            await parent.mark_job_rescued(session)
 
-        job4 = await parent.rescue_job(session)  # type: ignore
+        job4 = await parent.rescue_job(session)
         await db.Job.update_row(session, job4.id, status=StatusEnum.accepted)
 
         rescued = await interface.mark_job_rescued(session, parent.fullname)