pants: Pants dependencies don't respect skip_mypy flag when running typecheck.
Describe the bug
If you run ./pants typecheck xyz, it will be skipped for any tests or libraries in the module xyz that have the flag skip_mypy=True. However, if you reference a pants library as a dependency, it ignores the skip_mypy flag on that library.
For example, say we have two libraries: untyped and typed. untyped doesn’t have type hints yet (or even worse, incorrect type hints). It also has the skip_mypy=True flag set. typed has type hints, and has the tag typecheck in the library. The typed library depends on the untyped library, and imports some of it’s functions.
Expected behaviour: When running typecheck, it does not run on dependencies that have the skip_mypy flag.
Actual behaviour: Mypy is ran on all dependencies, whether or not the flag is set.
Pants version
2.6.1
OS
MacOS
Additional info
I set up a repo with a minimal representation of this problem: https://github.com/nicholas-plutoflume/pants_mypy_skip_bug
Simply clone it, and run ./pants typecheck typed::.
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 17 (10 by maintainers)
So this seems to be exactly the case described in https://mypy.readthedocs.io/en/stable/running_mypy.html#following-imports . I.e., this is how mypy is supposed to behave in this case.
But they do propose a solution, which is to pass
-follow-imports=silentto mypy. You can do this on the Pants command line with--mypy-args="--follow-imports=silent", and if that works, you can set it in pants.toml (theargskey in the[mypy]section).Or, you can set this in your
mypy.ini(see here for details) and Pants should auto-discover and use that file.Let us know if this works! On balance I would say that MyPy (and therefore Pants) is behaving as intended. But if this fix works we’d welcome a suggested edit to the relevant documentation page, so other people don’t get tripped up on this. Thanks!
Yes! That worked. I agree with your analysis, I’ll see if I can throw together a sentence for the documentation page.
Yes, I’m not saying that there’s not a bug here, just hoped to get you further along until there is a fix for it, if it is a bug.
Edit: expect a more authoritative answer within 6 hours or so…
Thanks! So this tells me that only
typedis on the mypy command line. So far so good. So I’m guessing that mypy is configured to fail when its upstream dependencies (untypedin this case) fail to check.What mypy error are you getting?
For example, if, as I suspect, mypy is erroring on upstream dependencies that don’t typecheck, then there may be a way to modify that behavior. Or, if not, then we may have to update the docs to point out that mypy doesn’t allow every combo of typed and untyped, and so even incremental adoption might have to be in dep order…
Hi @nicholas-plutoflume , thanks for the bug report. To elaborate on @kaos and @Eric-Arellano’s information -
So whether this is a bug or not really depends on how you ran Pants, and on what precisely you mean by “Mypy ran on all dependencies”. In your case, to typecheck
typed, mypy needs symbol information fromuntyped, so it has to be able to “look at it”.But, since it has
skip_mypyset then we shouldn’t be puttinguntypedon the mypy command line as a file for mypy to directly check. Rather it will be on the sys.path, so mypy can discover it as an upstream dependency. See here. So now the question is what does mypy do when an upstream dependency that it was not explicitly asked to check, fails to check.What @Eric-Arellano is asking for with the troubleshooting step above (re
--no-process-execution-local-cleanup) is to see how the mypy command line was constructed by Pants, and specifically whether that skipped library’s files are explicitly on the command line or not.So if you could, at your convenience, take a look at that and also post your pants cmd-line (suitably redacted if necessary), we can figure out the exact nature of the bug.
Thanks @kaos!
Hi @nicholas-plutoflume! I’m afk and about to leave for a flight so can’t check rn, but: MyPy requires that you have transitive dependencies in the build’s context (chroot/tmpdir), otherwise it will complain that the module is missing. That’s important for us to keep doing I think.
But, there is a difference in MyPy’s behavior if you say to explicitly run on the dependency vs only having it present in the chroot.
mypy app.pyis different thanmypy app.py dep.py. We have a test for this in mypy/rules_integration_test.py, but it would be good to confirm this is the case. I recommend running with--no-process-execution-local-cleanup, which will log the chroot used for MyPy and allow you to inspect what Pants is running - look at the __run.sh script. https://www.pantsbuild.org/docs/troubleshootingDefinitely open to your feedback on what Pants should be doing here, although I suspect the behavior is right and it’s our docs that need to be improved to explain this nuance. I suspect you’ll want to use MyPy to ignore the problematic lines like with
# type: ignore. https://mypy.readthedocs.io/en/stable/existing_code.html#start-small. Pants would still be doing a helpful thing by constructing the argv to not include the skipped files, although that only goes so far.Thanks for the tip, yeah, I have. That’s the approach I’m taking at the moment, but given the size of the codebase we wanted all new things to have typing, and eventually to add typing to existing modules.
Considering the
skip_mypyflag exists, I had expected it’s behaviour to be different, and to allow us to add typing to new things without needing to modify existing modules (yet).But hey, the faster we adopt types everywhere, the better 😄