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

upgrade mypy to latest #4486

Closed
wants to merge 8 commits into from
Closed

upgrade mypy to latest #4486

wants to merge 8 commits into from

Conversation

nathaniel-may
Copy link
Contributor

@nathaniel-may nathaniel-may commented Dec 14, 2021

Description

This PR Is stuck. I don't know what the path forward is.

From our version to this version we gain:

  • M1 wheels for developers
  • TypeGuard from 3.10 (and typing extensions) which can be used to replace some uses of isinstance for type narrowing
  • type narrowing with type() not just isinstance
  • significant performance improvements
  • adds the --exclude flag for specifying modules
  • official 3.9 support
  • many bug fixes

The two big issues with getting this to work:

  1. mypy now traverses projects differently, and needs changes to find all our source code.
  2. the mechanism our unit tests use to import core code is unknown and breaks when we fix issue number one.

Fix for 1: add empty __init__.py files to the following directories:

  • core/dbt/
  • core/dbt/adapters/
  • core/dbt/context/
  • core/dbt/deps/

Fix for 2: remove the __init__.py files from the following directories:

  • core/dbt/
  • core/dbt/adapters/

This means we have one of two things breaking at any given time. Either mypy fails because we it detects an error with the way we've named our modules because it's not traversing them all, or tests fail to import the postgres adapter from dbt.adapters.postgres. With 1 fixed, and 2 failing, dbt run still works after install, so it's likely tests are failing because of test setup when the only change that has been made is adding init files to the context and deps packages.

The reason the import mechanism is unknown is because the postgres adapter lives in the plugin directory. When you run pip install dbt-postgres, it installs dbt-core, then finds the postgres adapter in the plugins directory and copies those modules to the path dbt/core/adapters/postgres so they can be imported like from dbt.adapters.postgres import PostgresAdapter. When you manually inspect the virtual environment after doing a fresh pip install -r dev-requirements.txt -r editable-requirements.txt, which is what both the makefile and tox are doing, the adapter modules have not been moved to that location yet the tests still run and import the adapter just fine.

If we cannot find a path forward to make both mypy and unit tests pass, we may have to abandon this upgrade.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@nathaniel-may
Copy link
Contributor Author

Hmm. Looks like it's failing here, but not locally. I thought both configs were pulling from tox.ini. Am I missing something about our CI?

@gshank
Copy link
Contributor

gshank commented Dec 14, 2021

I got the failure locally too. You might need to remove the .tox mypy directory, run 'make dev' and then run 'make mypy'.

@nathaniel-may
Copy link
Contributor Author

Since this PR mypy has released v0.930. We should use that instead when we come back to this PR.

@gshank gshank marked this pull request as draft January 3, 2022 16:02
@kwigley kwigley mentioned this pull request Jan 3, 2022
4 tasks
@gshank gshank removed their request for review February 15, 2022 19:11
@kwigley kwigley removed their request for review February 22, 2022 19:00
@gshank
Copy link
Contributor

gshank commented Mar 6, 2022

We use "namespace packages" for our adapter directories (no init.py files), so we need to use the --namespace-packages flag. And possibly specify directories, I'm a little fuzzy on that. The init.py files are no longer required as of Python 3.3, but mypy doesn't support the lack of them all that well. More information:

python/mypy#1645

python/mypy#5759.

The adapters will not work with an init.py file in the dbt/adapters directory, because that makes dbt.adapters a package and prevents the adapters from being installed in dbt.adapters.postgres, etc.

@leahwicz
Copy link
Contributor

leahwicz commented May 9, 2022

@nathaniel-may can we close this PR out since we merged Ian's?

@nathaniel-may
Copy link
Contributor Author

closing in favor of #5171

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

Successfully merging this pull request may close these issues.

3 participants