Support V2 linters running on Python 2 and Python 3 targets at the same time

[Eric-Arellano]

Problem

When using the default --no-per-target-caching, ./v2 lint :: will fail if you have any targets with conflicting Python interpreter constraints, e.g. Python 2-only targets mixed with Python 3-only targets. This is a common problem due to Python 3 migrations.

It is not acceptable to say "just use --per-target-caching" because we want ./pants lint :: to work regardless of this setting.

Solution

The main change is for linter rules to now return LintResults, rather than a single LintResult. Typically, this will only have one element, but it can include more, such as partitioning by interpreter constraints. In JVM land, we might partition by JVM compatibility, for example.

Then, Pylint, Bandit, and Flake8 are rewritten to have new rules that take a Partition of the original input targets and return a single LintResult. Another rule does the partitioning and aggregates these results.

Result

./v2 lint :: works on any Python codebase, regardless of interpreter constraints.

[Eric-Arellano]

Apologies that this is large. I recommend going commit-by-commit.

[stuhood]

It is not acceptable to say "just use --per-target-caching" because the performance is so much worse on a cold cache.

Is this actually the case for the usecases we recommend? ie, --changed-parent=X? This is only one of many complexities added by the fmt/lint batching (as described in your doc), and we should be sure that it's worth continuing to support batching.

EDIT: And also whether it continues to be the case as we further optimize process execution via materialize_directory, etc.

[Eric-Arellano]

Is this actually the case for the usecases we recommend? ie, --changed-parent=X? This is only one of many complexities added by the fmt/lint batching (as described in your doc), and we should be sure that it's worth continuing to support batching.

Likely, some cases are faster with --changed-since --per-target-caching, but others are not. It's hard to reason about and depends on how many things have changed and your target granularity. FYI https://docs.google.com/document/d/1Tdof6jx9aVaOGeIQeI9Gn-8x7nd6LfjnJ0QrbLFBbgc/edit#heading=h.oyst624c2sph has the benchmarks.

But, overall, we decided to default to --no-per-target-caching because in most cases it has better performance. For new users, we don't want them to have to deal with the special cases of when to use --per-target-caching and when to not. It exists as a possible optimization, but you don't need to worry about if you don't want to.

For that first-time user, we want ./pants lint :: to work regardless of whether they're using --per-target-caching or not. This PR means that --per-target-caching becomes solely about differences in performance, rather than also impacting interpreter conflicts.

We can replace the warning in https://pants.readme.io/v1.28/docs/python-lint-goal with a new green "Benefits of Pants" box about how you can run your linters on incompatible targets.

[stuhood]

Change looks reasonable: thanks.

As discussed, I think that we should reconsider this default though. We would never recommend that someone should run ./pants lint ::, but if they did run it, more egregious than the first run taking a long time would be none of the subsequent runs doing any caching at all due to all of the files having been batched. People should rely on our caching behavior, and it's easily broken with batching. Even in the case of --changed-parent=master, folks are going to end up running these tools multiple times: those additional runs should mostly hit local caches.

[benjyw]

I still think the solution to the caching issue is to cache successes (and only successes) on per-file basis, after running in a single batch. Ideally there would be an intrinsic to poke data into the CAS without running a process (although we could start out by just caching the result of true, and sucking up the extra process execution hit).

[benjyw]

This should be straightforward because we're not caching any data that need to be split out, we're just caching the fact "this file passed lint".