JP-3749: Add mypy type checking

[emolter]

Resolves JP-3749

Closes #8794

This PR adds type hint checking to the GitHub Actions workflow. The mypy configuration is nearly identical to the one in stcal, except that it disables checking imports globally instead of module-by-module. In order to bring the repository up to the standard required by these mypy checks, this PR also:

• Removes type hinting from extract_1d
• Makes small modifications throughout the code, especially in places where file- or class-wide constants are set (mypy doesn't ignore these the way it ignores non-hinted functions) as well as in abstract base classes
• Sets the typing: ignore flag in a few places, hopefully judiciously and with good reason

Before all the mypy checks can pass, there are a few outstanding issues that need to be resolved:

• mypy doesn't like the fact that __all__ in jwst.datamodels gets lots of classes from stdatamodels.jwst.datamodels appended to it. I think the discussion in MyPy not acknowledging __all__  python/mypy#1606 may be relevant, but I can't figure out a good solution that doesn't involve manually ignoring all ~20 places in the repository where we import a specific model type from datamodels. Edit: I did decide to just manually ignore the 14 instances of this error. If someone has a better idea during PR review, I'm all ears.
• set_telescope_pointing was type-hinted at some point, but the hints are either outdated or were never checked by mypy. These look like a fair amount of work to fix - perhaps removing them entirely is the best plan. Need to figure out if there would be objections to that. Edit: After discussing with @stscieisenhamer, I removed all type hints not directly related to the two dataclasses in there, and then found a sneaky way to make mypy ok with setting many of the attributes to None. There may still be a better solution but this one seems to work, at least.

Tasks

• request a review from someone specific, to avoid making the maintainers review every PR
• add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
• Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
  • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • start a regression test and include a link to the running job (click here for instructions)
    • Do truth files need to be updated ("okified")?
      • after the reviewer has approved these changes, run okify_regtests to update the truth files
• if a JIRA ticket exists, make sure it is resolved properly

news fragment change types...

• changes/<PR#>.general.rst: infrastructure or miscellaneous change
• changes/<PR#>.docs.rst
• changes/<PR#>.stpipe.rst
• changes/<PR#>.datamodels.rst
• changes/<PR#>.scripts.rst
• changes/<PR#>.fits_generator.rst
• changes/<PR#>.set_telescope_pointing.rst
• changes/<PR#>.pipeline.rst

stage 1

• changes/<PR#>.group_scale.rst
• changes/<PR#>.dq_init.rst
• changes/<PR#>.emicorr.rst
• changes/<PR#>.saturation.rst
• changes/<PR#>.ipc.rst
• changes/<PR#>.firstframe.rst
• changes/<PR#>.lastframe.rst
• changes/<PR#>.reset.rst
• changes/<PR#>.superbias.rst
• changes/<PR#>.refpix.rst
• changes/<PR#>.linearity.rst
• changes/<PR#>.rscd.rst
• changes/<PR#>.persistence.rst
• changes/<PR#>.dark_current.rst
• changes/<PR#>.charge_migration.rst
• changes/<PR#>.jump.rst
• changes/<PR#>.clean_flicker_noise.rst
• changes/<PR#>.ramp_fitting.rst
• changes/<PR#>.gain_scale.rst

stage 2

• changes/<PR#>.assign_wcs.rst
• changes/<PR#>.badpix_selfcal.rst
• changes/<PR#>.msaflagopen.rst
• changes/<PR#>.nsclean.rst
• changes/<PR#>.imprint.rst
• changes/<PR#>.background.rst
• changes/<PR#>.extract_2d.rst
• changes/<PR#>.master_background.rst
• changes/<PR#>.wavecorr.rst
• changes/<PR#>.srctype.rst
• changes/<PR#>.straylight.rst
• changes/<PR#>.wfss_contam.rst
• changes/<PR#>.flatfield.rst
• changes/<PR#>.fringe.rst
• changes/<PR#>.pathloss.rst
• changes/<PR#>.barshadow.rst
• changes/<PR#>.photom.rst
• changes/<PR#>.pixel_replace.rst
• changes/<PR#>.resample_spec.rst
• changes/<PR#>.residual_fringe.rst
• changes/<PR#>.cube_build.rst
• changes/<PR#>.extract_1d.rst
• changes/<PR#>.resample.rst

stage 3

• changes/<PR#>.assign_mtwcs.rst
• changes/<PR#>.mrs_imatch.rst
• changes/<PR#>.tweakreg.rst
• changes/<PR#>.skymatch.rst
• changes/<PR#>.exp_to_source.rst
• changes/<PR#>.outlier_detection.rst
• changes/<PR#>.tso_photometry.rst
• changes/<PR#>.stack_refs.rst
• changes/<PR#>.align_refs.rst
• changes/<PR#>.klip.rst
• changes/<PR#>.spectral_leak.rst
• changes/<PR#>.source_catalog.rst
• changes/<PR#>.combine_1d.rst
• changes/<PR#>.ami.rst

other

• changes/<PR#>.wfs_combine.rst
• changes/<PR#>.white_light.rst
• changes/<PR#>.cube_skymatch.rst
• changes/<PR#>.engdb_tools.rst
• changes/<PR#>.guider_cds.rst

[codecov]

Codecov Report

Attention: Patch coverage is 96.21212% with 5 lines in your changes missing coverage. Please review.

Project coverage is 61.79%. Comparing base (fb3c2cd) to head (8764435).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/ami/leastsqnrm.py	0.00%	1 Missing ⚠️
jwst/extract_1d/apply_apcorr.py	93.33%	1 Missing ⚠️
jwst/lib/set_telescope_pointing.py	97.22%	1 Missing ⚠️
jwst/lib/siafdb.py	0.00%	1 Missing ⚠️
jwst/outlier_detection/tso.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8852      +/-   ##
==========================================
- Coverage   61.79%   61.79%   -0.01%     
==========================================
  Files         377      377              
  Lines       38834    38831       -3     
==========================================
- Hits        23999    23996       -3     
  Misses      14835    14835              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[emolter]

regression tests started here

[stscieisenhamer]

Regarding changes related to lib: LGTM

Leaving as just a comment since I did not do a full review.

[emolter]

Some failures in the first round of regtests related to over-zealous removal of things in extract1d, will look at this tomorrow or Monday and fix it

[emolter]

new run of regression tests here after fixing the bug locally

edit: all passing

[melanieclarke]

This does seem a bit sneaky. Am I misunderstanding, or is this allowed by mypy only because the NoneFactory itself is not marked with typing on its return value? PyCharm doesn't like the assignments to NoneFactory() because it looks like it does not have a return value.

Why not just modify the typing on the dataclass fields to be np.array | None, etc? I spot checked a few and it looks like mypy has no problem with that.

[emolter]

Yes that is the only reason it works.

I tried doing this to all of them, and it leads to several difficult-to-fix issues. I'll make a branch that shows these issues - it would certainly be nicer it we could actually fix them all, but it looked like a ton of work

[emolter]

@melanieclarke see here: https://github.com/spacetelescope/jwst/actions/runs/11262041086/job/31316829777?pr=8868
I played around a fair bit before trying to fix these, but I don't think it's very easy. I had a Slack discussion with @stscieisenhamer about this, and he confirmed that many of the parameters are needed in certain cases but not others, and also that it's not always simple to set a non-None default. It seemed likely to me that a pretty substantial refactor would be needed to fix this. What do you think?

[melanieclarke]

Maybe we should use the --no-strict-optional option? On your other branch, with this option, it has only one complaint about the set_telescope_pointing module.

[melanieclarke]

Or not. I just saw the warning that it is an evil option that should not be used.

[melanieclarke]

The other thing we can do is | Any instead of | None where needed.

I think the way you're doing it now is unnecessarily confusing, and it looks like it's effectively disabling the useful part of typing. This one, reported on your other branch, looks like a real code problem that's not caught in this branch:

jwst/lib/set_telescope_pointing.py:1047: error: Missing positional arguments "v2_ref", "v3_ref", "v3yangle", "vparity", "crpix1", "crpix2", "cdelt1", "cdelt2", "vertices_idl" in call to "SIAF"  [call-arg]

[emolter]

Using Any also defeats a lot of the utility of type hinting, but I agree in this case it may be better than what I did. And nice catch on that one error - this is indeed the only one that remains using Any types, which I assume is what you also tried locally.

There is functionally no difference between type | Any and simply Any, but I would propose to leave the pipes in there because it's more human-readable, i.e., "this should generally be type but we needed to allow Any to get mypy to shut up.

See the most recent commit where I did as you suggest and fixed the "missing positional arguments" bug

[melanieclarke]

Agreed, I like the pipes for readability. And also agreed that Any isn't super helpful as a type, but better than either a major refactor or just working around typing entirely.

Just one more suggestion here... You many not need | Any everywhere -- | None might be sufficient for some parameters. Trying it locally, I just added it to the arrays it was complaining about, until it stopped complaining.

[emolter]

done, lmk how it looks or any other comments you have on any of the changes, I know there are a lot

[melanieclarke]

Thank you! I think that looks good now.

I think everything else is fine, pending review from either Brett or Zach, but I'll take another glance through tomorrow, with fresh eyes.