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)

Most upvoted comments

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=silent to 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 (the args key 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 typed is on the mypy command line. So far so good. So I’m guessing that mypy is configured to fail when its upstream dependencies (untyped in 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 from untyped, so it has to be able to “look at it”.

But, since it has skip_mypy set then we shouldn’t be putting untyped on 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.py is different than mypy 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/troubleshooting

Definitely 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_mypy flag 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 😄