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

Include external srcs for mypy dep analysis with pip_install #43

Closed
wants to merge 2 commits into from
Closed

Include external srcs for mypy dep analysis with pip_install #43

wants to merge 2 commits into from

Conversation

jonmeow
Copy link

@jonmeow jonmeow commented Sep 13, 2021

At present, because external deps are skipped, requirement()-based imports don't work. This causes issues for python rules that are trying to use pip_install.

This makes a series of changes in order to enable requirement() compatibility:

  • Adds include_imports, defaulting False, to enable new behavior.
    • I'm cautious of causing issues for those who don't need/want this. In my personal repo, tests go from 5s to 50s -- parsing of the pip imports is definitely slow.
    • The flipside is that parsing imports makes this behave consistently for people who do or don't have the imports locally installed, and that consistency is why I'm proposing this change. That is, we'll accept the slowdown for consistency.
  • When extracting transitive deps, include_imports follows external and also gathers imports.
  • If imports were found, does work to make them relative to the workspace root.
    • A mypy_test //foo:bar would run from the foo directory; ../ is added to imports in that case so that they're found correctly. The resulting mypy path will look like ../py_deps/pypi__pygithub, allowing import github to resolve to ../py_deps/pypi__pygithub/github.
  • The target_src_files versus input_src_files split is done because target_src_files is the small set of project-owned files which are valid to examine, whereas input_src_files is used for runfiles generation and so needs to include imported files.
  • --follow-imports=silent is added to avoid printing errors on imported files.

This might fix #39, and may also help with #23 (although this may not interact correctly -- smarter import handling may be needed, this is really requirement()-focused).

jonmeow added a commit to carbon-language/carbon-lang that referenced this pull request Sep 13, 2021
I'm seeing if I can upstream bazel-contrib/bazel-mypy-integration#43, but we can also point at my fork for the time being.

This should resolve conflicts with mypy treating imports as non-hermetic, creating inconsistent behavior if packages are/aren't installed locally.
@alexeagle
Copy link
Collaborator

alexeagle commented May 6, 2022

This might fix #39

I patched this on top of the repro, but it's still red: https://github.com/bazel-contrib/bazel-mypy-integration/runs/6323849357

I think this is because it still doesn't place the stubs in a site-packages folder, which is the only place (?) mypy will look for it, as @rogerhub said in #39:

Mypy also offers a MYPYPATH, but it does not use the "...-stubs" layout, so we can't just stick all of the Bazel PyPI directories into the MYPYPATH (I tried that initially).

@jonmeow jonmeow closed this May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stubs from pypi are not found by mypy
2 participants