Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using mypy's ignore_missing_imports option #225

Closed
jtherrmann opened this issue Jan 16, 2025 · 2 comments
Closed

Stop using mypy's ignore_missing_imports option #225

jtherrmann opened this issue Jan 16, 2025 · 2 comments
Labels
Jira Bug Create a Jira Bug for this issue

Comments

@jtherrmann
Copy link
Contributor

jtherrmann commented Jan 16, 2025

Our v0.14.0 reusable-mypy action runs the following mypy command:

mypy --install-types --ignore-missing-imports --non-interactive --pretty .

I observe some unexpected behavior with --ignore-missing-imports and --install-types. Using burst2safe as an example, if I checkout the latest develop commit, build the mamba environment and run the above command, it installs types-python-dateutil-2.9.0.20241206 and flags no issues.

However, if I remove .mypy_cache/ and re-build the environment, then run the above command without --ignore-missing-imports, it installs lxml-stubs-0.5.1 types-Pygments-2.19.0.20250107 types-colorama-0.4.15.20240311 types-docutils-0.21.0.20241128 types-python-dateutil-2.9.0.20241206 types-setuptools-75.8.0.20250110 and then flags 200 errors in 28 files.

Some of these errors do not seem directly related to missing imports, e.g. src/burst2safe/safe.py:338: error: Cannot determine type of "size_bytes" for L338, or even if it is, that's not an error that we would want to ignore. I thought that particular error might be related to importing Kml via from burst2safe.manifest import Kml, Manifest, Preview at L14 because perhaps mypy does not recognize the burst2safe package, but installing the package via pip install -e . or using a relative import for that line both did not change the number of errors reported, so I'm not sure what's going on there.

Also, if I run the mypy command with only --ignore-missing-imports, then the only error it flags is:

src/burst2safe/burst_id.py:11: error: Library stubs not installed for "dateutil"  [import-untyped]

which I suppose makes some sense since that's the library for which stubs are installed when I run the full mypy command, though I'm not sure why --ignore-missing-imports doesn't suppress that particular import.

So, my main concerns are:

  • Why does using --ignore-missing-imports prevent installation of stubs for all libraries (other than dateutil)? I think we want to install as many stubs as possible.
  • Some of the ignored errors are not errors that we should be ignoring.

I think that the ideal resolution to this issue would be to:

  1. Remove --ignore-missing-imports from our reusable action, and avoid enabling it in pyproject.toml for new projects (will need to be added to pyproject.toml for existing projects where removing it would cause many errors).
  2. Gradually remove it from pyproject.toml for existing projects as time allows.
  3. Going forward, use more granular methods of ignoring specific imports as needed. See Missing imports.
@jtherrmann jtherrmann added the Jira Bug Create a Jira Bug for this issue label Jan 16, 2025
@jtherrmann jtherrmann changed the title Stop using mypy's ignore_missing_imports option globally Stop using mypy's ignore_missing_imports option for all modules Jan 17, 2025
@jtherrmann jtherrmann changed the title Stop using mypy's ignore_missing_imports option for all modules Stop using mypy's ignore_missing_imports option Jan 17, 2025
@jtherrmann
Copy link
Contributor Author

Actions v0.15.0 removes the cli options from the mypy action and recommends adding them to pyproject.toml.

It looks like the disable_error_code = ["import-untyped"] option might be the best solution for ignoring untyped imports, as shown at the bottom of this section of the docs, which also recommend against using --ignore-missing-imports. I'm not clear on the difference between these two options, but disable_error_code = ["import-untyped"] does not seem to have the undesired effects of --ignore-missing-imports.

When I added disable_error_code = ["import-untyped"] to burst2safe, running mypy . still installed lxml-stubs-0.5.1 types-Pygments-2.19.0.20250107 types-colorama-0.4.15.20240311 types-docutils-0.21.0.20241128 types-python-dateutil-2.9.0.20241206 types-setuptools-75.8.0.20250110 and then flagged 177 errors in 16 files, including the Cannot determine type of "size_bytes" described above. So, that's the behavior I would expect.

And when I added the option to https://github.com/ASFHyP3/OPERA-DISP-TMS, it allowed mypy . to succeed with no errors, because the only errors were import-untyped ones. Hopefully that will be the case for most of our repos, but we'll have to see.

I'll try using disable_error_code = ["import-untyped"] going forward, and add it to the mypy section of our README if it seems to be working as expected.

@jtherrmann
Copy link
Contributor Author

We've merged the latest actions version bump for all repos and replaced the --ignore-missing-imports option with disable_error_code = ["import-untyped"] in all of them. For those with minimal additional mypy errors that needed to be fixed as a result of this change, I made those fixes on the dependabot/github_actions/ASFHyP3/actions-0.15.0 branch and merged the PR without bothering anyone else for a review. For the few that required more substantial fixes, I opened separate PRs. I'll mark this issue done as soon as we've merged the last of the release PRs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jira Bug Create a Jira Bug for this issue
Projects
None yet
Development

No branches or pull requests

1 participant