Skip to content

Commit

Permalink
Rename --per-target-caching to --per-file-caching for lint and …
Browse files Browse the repository at this point in the history
…`fmt` (#10630)

Thanks to #10511, targets are no longer an atomic unit Pants runs on. Instead, files are the atomic unit.

This option rename reflects that model change.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Aug 17, 2020
1 parent daf80c4 commit b0bd5c3
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 51 deletions.
37 changes: 29 additions & 8 deletions src/python/pants/core/goals/fmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from dataclasses import dataclass
from typing import ClassVar, Iterable, List, Optional, Tuple, Type, cast

from pants.base.deprecated import resolve_conflicting_options
from pants.core.util_rules.filter_empty_sources import TargetsWithSources, TargetsWithSourcesRequest
from pants.engine.console import Console
from pants.engine.engine_aware import EngineAware
Expand Down Expand Up @@ -138,24 +139,44 @@ class FmtSubsystem(GoalSubsystem):
def register_options(cls, register) -> None:
super().register_options(register)
register(
"--per-target-caching",
"--per-file-caching",
advanced=True,
type=bool,
default=False,
help=(
"Rather than running all targets in a single batch, run each target as a "
"Rather than formatting all files in a single batch, format each file as a "
"separate process. Why do this? You'll get many more cache hits. Why not do this? "
"Formatters both have substantial startup overhead and are cheap to add one "
"additional file to the run. On a cold cache, it is much faster to use "
"`--no-per-target-caching`. We only recommend using `--per-target-caching` if you "
"`--no-per-file-caching`. We only recommend using `--per-file-caching` if you "
"are using a remote cache or if you have benchmarked that this option will be "
"faster than `--no-per-target-caching` for your use case."
"faster than `--no-per-file-caching` for your use case."
),
)
register(
"--per-target-caching",
advanced=True,
type=bool,
default=False,
help="See `--per-file-caching`.",
removal_version="2.1.0.dev0",
removal_hint=(
"Use the renamed `--per-file-caching` option instead. If this option is set, Pants "
"will now run per every file, rather than per target."
),
)

@property
def per_target_caching(self) -> bool:
return cast(bool, self.options.per_target_caching)
def per_file_caching(self) -> bool:
val = resolve_conflicting_options(
old_option="per_target_caching",
new_option="per_file_caching",
old_container=self.options,
new_container=self.options,
old_scope=self.name,
new_scope=self.name,
)
return cast(bool, val)


class Fmt(Goal):
Expand Down Expand Up @@ -202,7 +223,7 @@ async def fmt(
if language_targets_with_sources
)

if fmt_subsystem.per_target_caching:
if fmt_subsystem.per_file_caching:
per_language_results = await MultiGet(
Get(
LanguageFmtResults,
Expand Down Expand Up @@ -244,7 +265,7 @@ async def fmt(

# We group all results for the same formatter so that we can give one final status in the
# summary. This is only relevant if there were multiple results because of
# `--per-target-caching`.
# `--per-file-caching`.
formatter_to_results = defaultdict(set)
for result in individual_results:
formatter_to_results[result.formatter_name].add(result)
Expand Down
32 changes: 17 additions & 15 deletions src/python/pants/core/goals/fmt_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def run_fmt_rule(
language_target_collection_types: List[Type[LanguageFmtTargets]],
targets: List[Target],
result_digest: Digest,
per_target_caching: bool,
per_file_caching: bool,
include_sources: bool = True,
) -> str:
console = MockConsole(use_colors=False)
Expand All @@ -153,7 +153,9 @@ def run_fmt_rule(
rule_args=[
console,
Targets(targets),
create_goal_subsystem(FmtSubsystem, per_target_caching=per_target_caching),
create_goal_subsystem(
FmtSubsystem, per_file_caching=per_file_caching, per_target_caching=False
),
Workspace(self.scheduler),
union_membership,
],
Expand Down Expand Up @@ -193,40 +195,40 @@ def assert_workspace_modified(
assert smalltalk_file.read_text() == self.smalltalk_file.content.decode()

def test_empty_target_noops(self) -> None:
def assert_noops(*, per_target_caching: bool) -> None:
def assert_noops(*, per_file_caching: bool) -> None:
stderr = self.run_fmt_rule(
language_target_collection_types=[FortranTargets],
targets=[self.make_target()],
result_digest=self.fortran_digest,
per_target_caching=per_target_caching,
per_file_caching=per_file_caching,
include_sources=False,
)
assert stderr.strip() == ""
self.assert_workspace_modified(fortran_formatted=False, smalltalk_formatted=False)

assert_noops(per_target_caching=False)
assert_noops(per_target_caching=True)
assert_noops(per_file_caching=False)
assert_noops(per_file_caching=True)

def test_invalid_target_noops(self) -> None:
def assert_noops(*, per_target_caching: bool) -> None:
def assert_noops(*, per_file_caching: bool) -> None:
stderr = self.run_fmt_rule(
language_target_collection_types=[InvalidTargets],
targets=[self.make_target()],
result_digest=self.fortran_digest,
per_target_caching=per_target_caching,
per_file_caching=per_file_caching,
)
assert stderr.strip() == ""
self.assert_workspace_modified(fortran_formatted=False, smalltalk_formatted=False)

assert_noops(per_target_caching=False)
assert_noops(per_target_caching=True)
assert_noops(per_file_caching=False)
assert_noops(per_file_caching=True)

def test_summary(self) -> None:
"""Tests that the final summary is correct.
This checks that we:
* Merge multiple results for the same formatter together (when you use
`--per-target-caching`).
`--per-file-caching`).
* Correctly distinguish between skipped, changed, and did not change.
"""
fortran_addresses = [Address.parse(":f1"), Address.parse(":needs_formatting")]
Expand All @@ -239,12 +241,12 @@ def test_summary(self) -> None:
self.make_target(addr, target_cls=SmalltalkTarget) for addr in smalltalk_addresses
]

def assert_expected(*, per_target_caching: bool) -> None:
def assert_expected(*, per_file_caching: bool) -> None:
stderr = self.run_fmt_rule(
language_target_collection_types=[FortranTargets, SmalltalkTargets],
targets=[*fortran_targets, *smalltalk_targets],
result_digest=self.merged_digest,
per_target_caching=per_target_caching,
per_file_caching=per_file_caching,
)
self.assert_workspace_modified(fortran_formatted=True, smalltalk_formatted=True)
assert stderr == dedent(
Expand All @@ -256,8 +258,8 @@ def assert_expected(*, per_target_caching: bool) -> None:
"""
)

assert_expected(per_target_caching=False)
assert_expected(per_target_caching=True)
assert_expected(per_file_caching=False)
assert_expected(per_file_caching=True)


def test_streaming_output_skip() -> None:
Expand Down
47 changes: 34 additions & 13 deletions src/python/pants/core/goals/lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from dataclasses import dataclass
from typing import Iterable, Optional, Tuple, cast

from pants.base.deprecated import resolve_conflicting_options
from pants.core.goals.style_request import StyleRequest
from pants.core.util_rules.filter_empty_sources import (
FieldSetsWithSources,
Expand Down Expand Up @@ -149,18 +150,30 @@ class LintSubsystem(GoalSubsystem):
def register_options(cls, register) -> None:
super().register_options(register)
register(
"--per-target-caching",
"--per-file-caching",
advanced=True,
type=bool,
default=False,
help=(
"Rather than running all targets in a single batch, run each target as a "
"Rather than linting all files in a single batch, lint each file as a "
"separate process. Why do this? You'll get many more cache hits. Why not do this? "
"Linters both have substantial startup overhead and are cheap to add one "
"additional file to the run. On a cold cache, it is much faster to use "
"`--no-per-target-caching`. We only recommend using `--per-target-caching` if you "
"`--no-per-file-caching`. We only recommend using `--per-file-caching` if you "
"are using a remote cache or if you have benchmarked that this option will be "
"faster than `--no-per-target-caching` for your use case."
"faster than `--no-per-file-caching` for your use case."
),
)
register(
"--per-target-caching",
advanced=True,
type=bool,
default=False,
help="See `--per-file-caching`.",
removal_version="2.1.0.dev0",
removal_hint=(
"Use the renamed `--per-file-caching` option instead. If this option is set, Pants "
"will now run per every file, rather than per target."
),
)
register(
Expand All @@ -171,13 +184,21 @@ def register_options(cls, register) -> None:
advanced=True,
help=(
"Specifying a directory causes linters that support writing report files to write "
"into this directory.",
"into this directory."
),
)

@property
def per_target_caching(self) -> bool:
return cast(bool, self.options.per_target_caching)
def per_file_caching(self) -> bool:
val = resolve_conflicting_options(
old_option="per_target_caching",
new_option="per_file_caching",
old_container=self.options,
new_container=self.options,
old_scope=self.name,
new_scope=self.name,
)
return cast(bool, val)

@property
def reports_dir(self) -> Optional[str]:
Expand Down Expand Up @@ -215,8 +236,8 @@ async def lint(
if request
)

if lint_subsystem.per_target_caching:
all_per_target_results = await MultiGet(
if lint_subsystem.per_file_caching:
all_per_file_results = await MultiGet(
Get(LintResults, LintRequest, request.__class__([field_set]))
for request in valid_requests
for field_set in request.field_sets
Expand All @@ -225,12 +246,12 @@ async def lint(
all_results = tuple(
LintResults(
itertools.chain.from_iterable(
per_target_results.results for per_target_results in all_linter_results
per_file_results.results for per_file_results in all_linter_results
),
linter_name=linter_name,
)
for linter_name, all_linter_results in itertools.groupby(
all_per_target_results, key=lambda results: results.linter_name
all_per_file_results, key=lambda results: results.linter_name
)
)
else:
Expand All @@ -247,8 +268,8 @@ async def lint(
results.linter_name for results in all_results if len(results.reports) > 1
]
if linters_with_multiple_reports:
if lint_subsystem.per_target_caching:
suggestion = "Try running without `--lint-per-target-caching` set."
if lint_subsystem.per_file_caching:
suggestion = "Try running without `--lint-per-file-caching` set."
else:
suggestion = (
"The linters likely partitioned the input targets, such as grouping by Python "
Expand Down
32 changes: 17 additions & 15 deletions src/python/pants/core/goals/lint_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def run_lint_rule(
*,
lint_request_types: List[Type[LintRequest]],
targets: List[Target],
per_target_caching: bool,
per_file_caching: bool,
include_sources: bool = True,
) -> Tuple[int, str]:
console = MockConsole(use_colors=False)
Expand All @@ -121,7 +121,9 @@ def run_lint_rule(
console,
workspace,
Targets(targets),
create_goal_subsystem(LintSubsystem, per_target_caching=per_target_caching),
create_goal_subsystem(
LintSubsystem, per_file_caching=per_file_caching, per_target_caching=False
),
union_membership,
],
mock_gets=[
Expand All @@ -147,43 +149,43 @@ def run_lint_rule(
return result.exit_code, console.stderr.getvalue()

def test_empty_target_noops(self) -> None:
def assert_noops(per_target_caching: bool) -> None:
def assert_noops(per_file_caching: bool) -> None:
exit_code, stderr = self.run_lint_rule(
lint_request_types=[FailingRequest],
targets=[self.make_target()],
per_target_caching=per_target_caching,
per_file_caching=per_file_caching,
include_sources=False,
)
assert exit_code == 0
assert stderr == ""

assert_noops(per_target_caching=False)
assert_noops(per_target_caching=True)
assert_noops(per_file_caching=False)
assert_noops(per_file_caching=True)

def test_invalid_target_noops(self) -> None:
def assert_noops(per_target_caching: bool) -> None:
def assert_noops(per_file_caching: bool) -> None:
exit_code, stderr = self.run_lint_rule(
lint_request_types=[InvalidRequest],
targets=[self.make_target()],
per_target_caching=per_target_caching,
per_file_caching=per_file_caching,
)
assert exit_code == 0
assert stderr == ""

assert_noops(per_target_caching=False)
assert_noops(per_target_caching=True)
assert_noops(per_file_caching=False)
assert_noops(per_file_caching=True)

def test_summary(self) -> None:
"""Test that we render the summary correctly.
This tests that we:
* Merge multiple results belonging to the same linter (`--per-target-caching`).
* Merge multiple results belonging to the same linter (`--per-file-caching`).
* Decide correctly between skipped, failed, and succeeded.
"""
good_address = Address.parse(":good")
bad_address = Address.parse(":bad")

def assert_expected(*, per_target_caching: bool) -> None:
def assert_expected(*, per_file_caching: bool) -> None:
exit_code, stderr = self.run_lint_rule(
lint_request_types=[
ConditionallySucceedsRequest,
Expand All @@ -192,7 +194,7 @@ def assert_expected(*, per_target_caching: bool) -> None:
SuccessfulRequest,
],
targets=[self.make_target(good_address), self.make_target(bad_address)],
per_target_caching=per_target_caching,
per_file_caching=per_file_caching,
)
assert exit_code == FailingRequest.exit_code([bad_address])
assert stderr == dedent(
Expand All @@ -205,8 +207,8 @@ def assert_expected(*, per_target_caching: bool) -> None:
"""
)

assert_expected(per_target_caching=False)
assert_expected(per_target_caching=True)
assert_expected(per_file_caching=False)
assert_expected(per_file_caching=True)


def test_streaming_output_skip() -> None:
Expand Down

0 comments on commit b0bd5c3

Please sign in to comment.