Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add lifecycle handlers to allow extensions to hook session lifecycle #10988

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/python/pants/bin/local_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import logging
import os
from dataclasses import dataclass, replace
from typing import Mapping, Optional, Tuple
from typing import List, Mapping, Optional, Tuple

from pants.base.build_environment import get_buildroot
from pants.base.exception_sink import ExceptionSink
Expand All @@ -13,6 +13,7 @@
from pants.base.specs_parser import SpecsParser
from pants.base.workunit import WorkUnit
from pants.build_graph.build_configuration import BuildConfiguration
from pants.build_graph.lifecycle import SessionLifecycleHandler
from pants.core.util_rules.pants_environment import PantsEnvironment
from pants.engine.internals.native import Native
from pants.engine.internals.scheduler import ExecutionError
Expand Down Expand Up @@ -55,6 +56,7 @@ class LocalPantsRunner:
union_membership: UnionMembership
profile_path: Optional[str]
_run_tracker: RunTracker
_session_lifecycle_handlers: List[SessionLifecycleHandler]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be Tuple[SessionLifecycleHandler, ...] so that it's hashable. Even though the dataclass isn't frozen, we always bias towards immutability.


@classmethod
def parse_options(
Expand Down Expand Up @@ -162,6 +164,16 @@ def create(

profile_path = env.get("PANTS_PROFILE")

# Query registered extension lifecycle handlers to see if any wish to receive lifecycle events
# for this session.
session_lifecycle_handlers = []
for lifecycle_handler in build_config.lifecycle_handlers:
session_lifecycle_handler = lifecycle_handler.on_session_create(
build_root=build_root, options=options, specs=specs
)
if session_lifecycle_handler:
session_lifecycle_handlers.append(session_lifecycle_handler)
Comment on lines +171 to +175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See lifecycle.py for how I recommend changing the modeling of this all. Here, you would check if instance(handler, SessionLifecycleHandler), then call handler.create().


return cls(
build_root=build_root,
options=options,
Expand All @@ -170,6 +182,7 @@ def create(
graph_session=graph_session,
union_membership=union_membership,
profile_path=profile_path,
_session_lifecycle_handlers=session_lifecycle_handlers,
_run_tracker=RunTracker.global_instance(),
)

Expand Down Expand Up @@ -274,10 +287,19 @@ def run(self, start_time: float) -> ExitCode:
return help_printer.print_help()

with streaming_reporter.session():
# Invoke the session lifecycle handlers, if any.
for session_lifecycle_handler in self._session_lifecycle_handlers:
session_lifecycle_handler.on_session_start()

engine_result = PANTS_FAILED_EXIT_CODE
try:
engine_result = self._run_v2()
except Exception as e:
ExceptionSink.log_exception(e)
run_tracker_result = self._finish_run(engine_result)

# Invoke the session lifecycle handlers, if any.
for session_lifecycle_handler in self._session_lifecycle_handlers:
session_lifecycle_handler.on_session_end(engine_result=engine_result)

return self._merge_exit_codes(engine_result, run_tracker_result)
25 changes: 25 additions & 0 deletions src/python/pants/build_graph/build_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from typing import Any, Dict, Type, Union

from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.build_graph.lifecycle import ExtensionLifecycleHandler
from pants.engine.rules import Rule, RuleIndex
from pants.engine.target import Target
from pants.engine.unions import UnionRule
Expand All @@ -26,6 +27,7 @@ class BuildConfiguration:
rules: FrozenOrderedSet[Rule]
union_rules: FrozenOrderedSet[UnionRule]
target_types: FrozenOrderedSet[Type[Target]]
lifecycle_handlers: FrozenOrderedSet[ExtensionLifecycleHandler]

@dataclass
class Builder:
Expand All @@ -35,6 +37,9 @@ class Builder:
_rules: OrderedSet = field(default_factory=OrderedSet)
_union_rules: OrderedSet = field(default_factory=OrderedSet)
_target_types: OrderedSet[Type[Target]] = field(default_factory=OrderedSet)
_lifecycle_handlers: OrderedSet[ExtensionLifecycleHandler] = field(
default_factory=OrderedSet
)

def registered_aliases(self) -> BuildFileAliases:
"""Return the registered aliases exposed in BUILD files.
Expand Down Expand Up @@ -161,6 +166,25 @@ def register_target_types(
)
self._target_types.update(target_types)

def register_lifecycle_handlers(
self, handlers: Union[typing.Iterable[ExtensionLifecycleHandler], typing.Any]
):
if not isinstance(handlers, Iterable):
raise TypeError(
f"The entrypoint `lifecycle_handlers` must return an iterable. Given {repr(handlers)}"
)
bad_elements = [
handler
for handler in handlers
if not isinstance(handler, ExtensionLifecycleHandler)
]
if bad_elements:
raise TypeError(
"Every element of the entrypoint `lifecycle_handlers` must be a subclass of "
f"{ExtensionLifecycleHandler.__name__}. Bad elements: {bad_elements}."
)
self._lifecycle_handlers.update(handlers)

def create(self) -> "BuildConfiguration":
registered_aliases = BuildFileAliases(
objects=self._exposed_object_by_alias.copy(),
Expand All @@ -172,4 +196,5 @@ def create(self) -> "BuildConfiguration":
rules=FrozenOrderedSet(self._rules),
union_rules=FrozenOrderedSet(self._union_rules),
target_types=FrozenOrderedSet(self._target_types),
lifecycle_handlers=FrozenOrderedSet(self._lifecycle_handlers),
)
32 changes: 32 additions & 0 deletions src/python/pants/build_graph/build_configuration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import unittest
from typing import Optional

from pants.base.specs import Specs
from pants.build_graph.build_configuration import BuildConfiguration
from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.build_graph.lifecycle import ExtensionLifecycleHandler, SessionLifecycleHandler
from pants.engine.unions import UnionRule, union
from pants.option.options import Options
from pants.util.frozendict import FrozenDict
from pants.util.ordered_set import FrozenOrderedSet

Expand Down Expand Up @@ -64,3 +68,31 @@ class B:
self.bc_builder.register_rules([union_a])
self.bc_builder.register_rules([union_b])
assert self.bc_builder.create().union_rules == FrozenOrderedSet([union_a, union_b])

def test_register_lifecycle_handlers(self) -> None:
class FooHandler(ExtensionLifecycleHandler):
def on_session_create(
self,
*,
build_root: str,
options: Options,
specs: Specs,
) -> Optional[SessionLifecycleHandler]:
return None

class BarHandler(ExtensionLifecycleHandler):
def on_session_create(
self,
*,
build_root: str,
options: Options,
specs: Specs,
) -> Optional[SessionLifecycleHandler]:
return None

self.bc_builder.register_lifecycle_handlers([FooHandler()])
self.bc_builder.register_lifecycle_handlers([BarHandler()])
bc = self.bc_builder.create()
handlers = list(bc.lifecycle_handlers)
assert isinstance(handlers[0], FooHandler)
assert isinstance(handlers[1], BarHandler)
23 changes: 23 additions & 0 deletions src/python/pants/build_graph/lifecycle.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably rename the file to lifecycle_handler.py.

# Licensed under the Apache License, Version 2.0 (see LICENSE).
from typing import Optional

from pants.base.exiter import ExitCode
from pants.base.specs import Specs
from pants.option.options import Options


class SessionLifecycleHandler:
Copy link
Contributor

@Eric-Arellano Eric-Arellano Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you envision multiple types of LifecycleHandlers, I recommend a LifecycleHandler superclass. It's fine if it's empty for now, perhaps subclass ABC to express intent that it's abstract. We can fill the methods in as we add more use cases and the common interface emerges from usage.

I think you can delete ExtensionLifecycleHandler. BuildConfiguration should store a bunch of LifecycleHandler, where each is a certain concrete subclass. Then, we can call isinstance(SessionLifecycleHandler) etc to determine which type of handler we're dealing with in our local_pants_runner.py code.

def on_session_start(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A basic docstring would be helpful for both of these. Even though we're not documenting on the website yet, it will help future contributors to understand what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be fine to call these start() and end(), as the SessionLifecycleHandler already expresses when those events happen.

That will make it easier for us to factor the methods up into the general LifecycleHandler if/when we're ready to generalize. I can imagine the interface being create(), start(), and end(), and having subclasses like SessionLifecycleHandler vs. RuleInvocationLifecycleHandler.

pass

def on_session_end(self, *, engine_result: ExitCode):
pass


class ExtensionLifecycleHandler:
# Returns a SessionLifecycleHandler that will receive lifecycle events for the session.
def on_session_create(
self, *, build_root: str, options: Options, specs: Specs
) -> Optional[SessionLifecycleHandler]:
pass
Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can move this into SessionLifecycleHandler and call it create(). No longer return Optional; we already validated it's a SessionLifecycleHandler, so we know the creation should not fail.

Also it should be a classmethod, as it's a factory method to create a new instance.

6 changes: 6 additions & 0 deletions src/python/pants/init/extension_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ def load_plugins(
if "rules" in entries:
rules = entries["rules"].load()()
build_configuration.register_rules(rules)
if "lifecycle_handlers" in entries:
lifecycle_handlers = entries["lifecycle_handlers"].load()()
build_configuration.register_lifecycle_handlers(lifecycle_handlers)
loaded[dist.as_requirement().key] = dist


Expand Down Expand Up @@ -151,3 +154,6 @@ def invoke_entrypoint(name):
rules = invoke_entrypoint("rules")
if rules:
build_configuration.register_rules(rules)
lifecycle_handlers = invoke_entrypoint("lifecycle_handlers")
if lifecycle_handlers:
build_configuration.register_lifecycle_handlers(lifecycle_handlers)