Skip to content

Commit

Permalink
Remove PANTS(C)_PROFILE in favor of recommending py-spy. (pantsbu…
Browse files Browse the repository at this point in the history
…ild#16226)

`py-spy` is superior to `cProfile` based profiling, and is already [documented](https://github.com/pantsbuild/pants/blob/99e2130deec1ad87a417cdc997a5fecd3f458965/docs/markdown/Contributions/development/contributions-debugging.md#profiling-with-py-spy). Remove the `PANTS(C)_PROFILE` environment variables which used to trigger profiling (which are _not_ documented).

Fixes pantsbuild#3431.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
stuhood authored and jyggen committed Jul 27, 2022
1 parent ce8a88f commit e69ca6b
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 98 deletions.
69 changes: 31 additions & 38 deletions src/python/pants/bin/local_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
from pants.option.global_options import DynamicRemoteOptions, DynamicUIRenderer
from pants.option.options import Options
from pants.option.options_bootstrapper import OptionsBootstrapper
from pants.util.contextutil import maybe_profiled
from pants.util.logging import LogLevel

logger = logging.getLogger(__name__)
Expand All @@ -46,7 +45,6 @@ class LocalPantsRunner:
run_tracker: A tracker for metrics for the run.
specs: The specs for this run, i.e. either the address or filesystem specs.
graph_session: A LegacyGraphSession instance for graph reuse.
profile_path: The profile path - if any (from from the `PANTS_PROFILE` env var).
"""

options: Options
Expand All @@ -56,7 +54,6 @@ class LocalPantsRunner:
specs: Specs
graph_session: GraphSession
union_membership: UnionMembership
profile_path: str | None

@classmethod
def _init_graph_session(
Expand Down Expand Up @@ -162,8 +159,6 @@ def create(
session=graph_session.scheduler_session,
)

profile_path = env.get("PANTS_PROFILE")

return cls(
options=options,
options_bootstrapper=options_bootstrapper,
Expand All @@ -172,7 +167,6 @@ def create(
specs=specs,
graph_session=graph_session,
union_membership=union_membership,
profile_path=profile_path,
)

def _perform_run(self, goals: tuple[str, ...]) -> ExitCode:
Expand Down Expand Up @@ -246,35 +240,34 @@ def _run_inner(self) -> ExitCode:
return PANTS_FAILED_EXIT_CODE

def run(self, start_time: float) -> ExitCode:
with maybe_profiled(self.profile_path):
spec_parser = SpecsParser()
specs = []
for spec_str in self.options.specs:
spec, is_ignore = spec_parser.parse_spec(spec_str)
specs.append(f"-{spec}" if is_ignore else str(spec))

self.run_tracker.start(run_start_time=start_time, specs=specs)
global_options = self.options.for_global_scope()

streaming_reporter = StreamingWorkunitHandler(
self.graph_session.scheduler_session,
run_tracker=self.run_tracker,
specs=self.specs,
options_bootstrapper=self.options_bootstrapper,
callbacks=self._get_workunits_callbacks(),
report_interval_seconds=global_options.streaming_workunits_report_interval,
allow_async_completion=(
global_options.pantsd and global_options.streaming_workunits_complete_async
),
max_workunit_verbosity=global_options.streaming_workunits_level,
)
with streaming_reporter:
engine_result = PANTS_FAILED_EXIT_CODE
try:
engine_result = self._run_inner()
finally:
metrics = self.graph_session.scheduler_session.metrics()
self.run_tracker.set_pantsd_scheduler_metrics(metrics)
self.run_tracker.end_run(engine_result)

return engine_result
spec_parser = SpecsParser()
specs = []
for spec_str in self.options.specs:
spec, is_ignore = spec_parser.parse_spec(spec_str)
specs.append(f"-{spec}" if is_ignore else str(spec))

self.run_tracker.start(run_start_time=start_time, specs=specs)
global_options = self.options.for_global_scope()

streaming_reporter = StreamingWorkunitHandler(
self.graph_session.scheduler_session,
run_tracker=self.run_tracker,
specs=self.specs,
options_bootstrapper=self.options_bootstrapper,
callbacks=self._get_workunits_callbacks(),
report_interval_seconds=global_options.streaming_workunits_report_interval,
allow_async_completion=(
global_options.pantsd and global_options.streaming_workunits_complete_async
),
max_workunit_verbosity=global_options.streaming_workunits_level,
)
with streaming_reporter:
engine_result = PANTS_FAILED_EXIT_CODE
try:
engine_result = self._run_inner()
finally:
metrics = self.graph_session.scheduler_session.metrics()
self.run_tracker.set_pantsd_scheduler_metrics(metrics)
self.run_tracker.end_run(engine_result)

return engine_result
1 change: 0 additions & 1 deletion src/python/pants/bin/pants_env_vars.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@
IGNORE_UNRECOGNIZED_ENCODING = "PANTS_IGNORE_UNRECOGNIZED_ENCODING"
RECURSION_LIMIT = "PANTS_RECURSION_LIMIT"
DAEMON_ENTRYPOINT = "PANTS_DAEMON_ENTRYPOINT"
PANTSC_PROFILE = "PANTSC_PROFILE"
17 changes: 7 additions & 10 deletions src/python/pants/bin/pants_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@
from pants.bin.pants_env_vars import (
DAEMON_ENTRYPOINT,
IGNORE_UNRECOGNIZED_ENCODING,
PANTSC_PROFILE,
RECURSION_LIMIT,
)
from pants.bin.pants_runner import PantsRunner
from pants.util.contextutil import maybe_profiled


class PantsLoader:
Expand Down Expand Up @@ -86,14 +84,13 @@ def run_alternate_entrypoint(entrypoint: str) -> None:

@staticmethod
def run_default_entrypoint() -> None:
with maybe_profiled(os.environ.get(PANTSC_PROFILE)):
start_time = time.time()
try:
runner = PantsRunner(args=sys.argv, env=os.environ)
exit_code = runner.run(start_time)
except KeyboardInterrupt as e:
print(f"Interrupted by user:\n{e}", file=sys.stderr)
exit_code = PANTS_FAILED_EXIT_CODE
start_time = time.time()
try:
runner = PantsRunner(args=sys.argv, env=os.environ)
exit_code = runner.run(start_time)
except KeyboardInterrupt as e:
print(f"Interrupted by user:\n{e}", file=sys.stderr)
exit_code = PANTS_FAILED_EXIT_CODE
sys.exit(exit_code)

@classmethod
Expand Down
1 change: 0 additions & 1 deletion src/python/pants/testutil/pants_integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ def run_pants_with_workdir_without_waiting(
for h in (
"HOME",
"PATH", # Needed to find Python interpreters and other binaries.
"PANTS_PROFILE",
):
value = os.getenv(h)
if value is not None:
Expand Down
32 changes: 0 additions & 32 deletions src/python/pants/util/contextutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

from __future__ import annotations

import logging
import os
import shutil
import ssl
Expand All @@ -17,8 +16,6 @@
from socketserver import TCPServer
from typing import IO, Any, Callable, Iterator, Mapping

from colors import green

from pants.util.dirutil import safe_delete


Expand Down Expand Up @@ -247,35 +244,6 @@ def open_zip(path_or_file: str | Any, *args, **kwargs) -> Iterator[zipfile.ZipFi
zf.close()


@contextmanager
def maybe_profiled(profile_path: str | None) -> Iterator[None]:
"""A profiling context manager.
:param profile_path: The path to write profile information to. If `None`, this will no-op.
"""
if not profile_path:
yield
return

import cProfile

profiler = cProfile.Profile()
try:
profiler.enable()
yield
finally:
profiler.disable()
profiler.dump_stats(profile_path)
view_cmd = green(
"gprof2dot -f pstats {path} | dot -Tpng -o {path}.png && open {path}.png".format(
path=profile_path
)
)
logging.getLogger().info(
f"Dumped profile data to: {profile_path}\nUse e.g. {view_cmd} to render and view."
)


@contextmanager
def http_server(handler_class: type, ssl_context: ssl.SSLContext | None = None) -> Iterator[int]:
def serve(port_queue: Queue[int], shutdown_queue: Queue[bool]) -> None:
Expand Down
16 changes: 0 additions & 16 deletions src/python/pants/util/contextutil_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import os
import pstats
import shutil
import subprocess
import sys
Expand All @@ -16,7 +15,6 @@
InvalidZipPath,
environment_as,
hermetic_environment_as,
maybe_profiled,
open_zip,
pushd,
temporary_dir,
Expand Down Expand Up @@ -199,17 +197,3 @@ def test_permissions(self) -> None:

with temporary_dir(permissions=0o644) as path:
assert 0o644 == os.stat(path)[0] & 0o777

def test_maybe_profiled(self) -> None:
with temporary_dir() as td:
profile_path = os.path.join(td, "profile.prof")

with maybe_profiled(profile_path):
for _ in range(5):
print("test")

# Ensure the profile data was written.
assert os.path.exists(profile_path)

# Ensure the profile data is valid.
pstats.Stats(profile_path).print_stats()

0 comments on commit e69ca6b

Please sign in to comment.