Skip to content

Commit

Permalink
Remove redundant names and logging. (#9921)
Browse files Browse the repository at this point in the history
### Problem

Post #9894, it's clear that many named rules are redundant, because the processes that run below them have better descriptions (relates to #7907, although it's possible that we'd like to drive the names of a workunit in some other way).

Example for a `--no-process-execution-use-local-cache` run:
```
18:00:22:852 [INFO] Starting tests: tests/python/pants_test/util:strutil
18:00:22:870 [INFO] Completed: Create a PEX from targets
18:00:24:029 [INFO] Completed: Building test_runner.pex
18:00:24:030 [INFO] Completed: Building requirements.pex
18:00:24:031 [INFO] Completed: Create PEX
18:00:24:031 [INFO] Completed: Create PEX
18:00:32:638 [INFO] Completed: Building pytest.pex with 8 requirements: ipdb, pygments, <snip>
18:00:32:639 [INFO] Completed: Create PEX
18:00:34:480 [INFO] Completed: Run Pytest for tests/python/pants_test/util:strutil
18:00:34:480 [INFO] Completed: Run pytest
18:00:34:481 [INFO] Tests succeeded: tests/python/pants_test/util:strutil
✓ tests/python/pants_test/util:strutil
============================= test session starts ==============================
<snip>
============================== 6 passed in 0.03s ===============================

18:00:34:481 [INFO] Completed: `test` goal
```

Likewise, when the process cache is hit on a run without `pantsd`, the `@rules` will run ("Create PEX" etc) but the processes won't because they'll have been persistently cached... which makes the output even less useful.

### Solution

While we should pursue #7907, and likely also allow `@rule`s to explicitly declare their level, some rule names just won't be useful when compared to the process names we've generated. Prune a few.

### Result

Output for the same uncached command is:
```
00:35:51:820 [INFO] Completed: Building requirements.pex
00:35:51:821 [INFO] Completed: Building test_runner.pex
00:36:02:208 [INFO] Completed: Building pytest.pex with 8 requirements: ipdb, pygments, <snip>
00:36:04:487 [INFO] Completed: Run Pytest for tests/python/pants_test/util:strutil
00:36:04:488 [INFO] Tests succeeded: tests/python/pants_test/util:strutil
✓ tests/python/pants_test/util:strutil
============================= test session starts ==============================
<snip>
============================== 6 passed in 0.03s ===============================

18:05:23:318 [INFO] Completed: `test` goal
```

See also: the travis output for this run.

[ci skip-jvm-tests]
[ci skip-rust-tests]
  • Loading branch information
stuhood authored Jun 1, 2020
1 parent c5cc29f commit 8831481
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 11 deletions.
4 changes: 2 additions & 2 deletions src/python/pants/backend/python/rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
)
from pants.engine.platform import Platform, PlatformConstraint
from pants.engine.process import MultiPlatformProcess, ProcessResult
from pants.engine.rules import RootRule, SubsystemRule, named_rule, rule
from pants.engine.rules import RootRule, SubsystemRule, rule
from pants.engine.selectors import Get
from pants.python.python_repos import PythonRepos
from pants.python.python_setup import PythonSetup
Expand Down Expand Up @@ -304,7 +304,7 @@ def log(self, *args, **kwargs) -> None:
self.log_level.log(logger, *args, **kwargs)


@named_rule(desc="Create PEX")
@rule
async def create_pex(
request: PexRequest,
pex_bin: DownloadedPexBin,
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/python/rules/pex_from_targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
)
from pants.engine.addresses import Addresses
from pants.engine.fs import Digest, MergeDigests
from pants.engine.rules import RootRule, named_rule, rule
from pants.engine.rules import RootRule, rule
from pants.engine.selectors import Get
from pants.engine.target import Targets, TransitiveTargets
from pants.python.python_setup import PythonSetup
Expand Down Expand Up @@ -85,7 +85,7 @@ class TwoStepPexFromTargetsRequest:
pex_from_targets_request: PexFromTargetsRequest


@named_rule(desc="Create a PEX from targets")
@rule
async def pex_from_targets(request: PexFromTargetsRequest, python_setup: PythonSetup) -> PexRequest:
transitive_targets = await Get[TransitiveTargets](Addresses, request.addresses)
all_targets = transitive_targets.closure
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/rules/pytest_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ async def setup_pytest_for_target(
)


@named_rule(desc="Run pytest")
@rule
async def run_python_test(
field_set: PythonTestFieldSet,
test_setup: TestTargetSetup,
Expand Down
8 changes: 2 additions & 6 deletions src/python/pants/core/goals/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
)
from pants.engine.unions import UnionMembership, union

# TODO(#6004): use proper Logging singleton, rather than static logger.
# TODO: Until we have templating of rule names (#7907) or some other way to affect the level
# of a workunit for a failed test, we should continue to log tests completing.
logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -311,11 +312,6 @@ async def run_tests(
@rule
async def coordinator_of_tests(wrapped_field_set: WrappedTestFieldSet) -> AddressAndTestResult:
field_set = wrapped_field_set.field_set

# TODO(#6004): when streaming to live TTY, rely on V2 UI for this information. When not a
# live TTY, periodically dump heavy hitters to stderr. See
# https://github.com/pantsbuild/pants/issues/6004#issuecomment-492699898.
logger.info(f"Starting tests: {field_set.address.reference()}")
result = await Get[TestResult](TestFieldSet, field_set)
logger.info(
f"Tests {'succeeded' if result.status == Status.SUCCESS else 'failed'}: "
Expand Down

0 comments on commit 8831481

Please sign in to comment.