Skip to content

Commit

Permalink
Use option value derivation from native parser. (#21627)
Browse files Browse the repository at this point in the history
The derivation (previously inaptly called "history" on the python side)
is displayed to users in option help. It shows what the option value
provided by each source type (config/env/flag) was, as well as the final
value (from the highest ranked source).

For scalar values this is relatively simple, as values can only be
overridden. For list and dict values this is more complicated,
as values from one source can be appended to by another source. 
So a final value (or any intermediate value) may come from multiple
sources.

The goal in this PR was to be compatible with the existing help
system tests, so we can be confident that we have equivalent
behavior. Once the legacy parser is gone we can consider
streamlining parts of this.
  • Loading branch information
benjyw authored Nov 11, 2024
1 parent 0d139ea commit 34f113a
Show file tree
Hide file tree
Showing 11 changed files with 427 additions and 113 deletions.
24 changes: 15 additions & 9 deletions src/python/pants/engine/internals/native_engine.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -659,11 +659,16 @@ class PyOptionId:
class PyConfigSource:
def __init__(self, path: str, content: bytes) -> None: ...

# See src/rust/engine/src/externs/options.rs for the Rust-side versions of these types.
T = TypeVar("T")
# A pair of (option value, rank). See src/python/pants/option/ranked_value.py.
OptionValue = Tuple[Optional[T], int]
OptionListValue = Tuple[list[T], int]
OptionDictValue = Tuple[dict[str, Any], int]

# List of pairs of (value, ranks of the sources of the value).
# A scalar value will always have a single source. A list/dict value
# may come from multiple sources, if its elements were appended.
OptionValueDerivation = list[Tuple[T, list[int]]]

# A tuple (value, rank of value, optional derivation of value).
OptionValue = Tuple[Optional[T], int, Optional[OptionValueDerivation]]

class PyOptionParser:
def __init__(
Expand All @@ -672,22 +677,23 @@ class PyOptionParser:
env: dict[str, str],
configs: Optional[Sequence[PyConfigSource]],
allow_pantsrc: bool,
include_derivation: bool,
) -> None: ...
def get_bool(self, option_id: PyOptionId, default: Optional[bool]) -> OptionValue[bool]: ...
def get_int(self, option_id: PyOptionId, default: Optional[int]) -> OptionValue[int]: ...
def get_float(self, option_id: PyOptionId, default: Optional[float]) -> OptionValue[float]: ...
def get_string(self, option_id: PyOptionId, default: Optional[str]) -> OptionValue[str]: ...
def get_bool_list(
self, option_id: PyOptionId, default: list[bool]
) -> OptionListValue[bool]: ...
def get_int_list(self, option_id: PyOptionId, default: list[int]) -> OptionListValue[int]: ...
) -> OptionValue[list[bool]]: ...
def get_int_list(self, option_id: PyOptionId, default: list[int]) -> OptionValue[list[int]]: ...
def get_float_list(
self, option_id: PyOptionId, default: list[float]
) -> OptionListValue[float]: ...
) -> OptionValue[list[float]]: ...
def get_string_list(
self, option_id: PyOptionId, default: list[str]
) -> OptionListValue[str]: ...
def get_dict(self, option_id: PyOptionId, default: dict[str, Any]) -> OptionDictValue: ...
) -> OptionValue[list[str]]: ...
def get_dict(self, option_id: PyOptionId, default: dict[str, Any]) -> OptionValue[dict]: ...
def get_passthrough_args(self) -> Optional[list[str]]: ...
def get_unconsumed_flags(self) -> dict[str, list[str]]: ...

Expand Down
26 changes: 18 additions & 8 deletions src/python/pants/help/help_formatter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
from __future__ import annotations

from dataclasses import replace
from typing import Tuple

from pants.help.help_formatter import HelpFormatter
from pants.help.help_info_extracter import HelpInfoExtracter, OptionHelpInfo
from pants.option.config import Config
from pants.option.global_options import GlobalOptions
from pants.option.native_options import NativeOptionParser
from pants.option.option_value_container import OptionValueContainerBuilder
from pants.option.parser import OptionValueHistory, Parser
from pants.option.ranked_value import Rank, RankedValue
Expand Down Expand Up @@ -58,43 +60,51 @@ def test_format_help_choices(self) -> None:
assert default_line.lstrip() == "default: kiwi"

@classmethod
def _get_parser(cls) -> Parser:
def _get_parser(cls) -> Tuple[Parser, NativeOptionParser]:
return Parser(
env={},
config=Config.load([]),
scope_info=GlobalOptions.get_scope_info(),
), NativeOptionParser(
args=[], env={}, config_sources=[], allow_pantsrc=False, include_derivation=True
)

@classmethod
def _format_for_global_scope(
cls, show_advanced: bool, show_deprecated: bool, args: list[str], kwargs
) -> list[str]:
parser = cls._get_parser()
parser, native_parser = cls._get_parser()
parser.register(*args, **kwargs)
return cls._format_for_global_scope_with_parser(
parser, show_advanced=show_advanced, show_deprecated=show_deprecated
parser, native_parser, show_advanced=show_advanced, show_deprecated=show_deprecated
)

@classmethod
def _format_for_global_scope_with_parser(
cls, parser: Parser, show_advanced: bool, show_deprecated: bool
cls,
parser: Parser,
native_parser: NativeOptionParser,
show_advanced: bool,
show_deprecated: bool,
) -> list[str]:
# Force a parse to generate the derivation history.
parser.parse_args(Parser.ParseArgsRequest((), OptionValueContainerBuilder(), [], False))
oshi = HelpInfoExtracter("").get_option_scope_help_info("", parser, False, "help.test")
oshi = HelpInfoExtracter("").get_option_scope_help_info(
"", parser, native_parser, False, "help.test"
)
return HelpFormatter(
show_advanced=show_advanced, show_deprecated=show_deprecated, color=False
).format_options(oshi)

def test_suppress_advanced(self) -> None:
parser = self._get_parser()
parser, native_parser = self._get_parser()
parser.register("--foo", advanced=True)
# must have a non advanced option to be able to supress showing advanced options.
parser.register("--jerry", advanced=False)
lines = self._format_for_global_scope_with_parser(parser, False, False)
lines = self._format_for_global_scope_with_parser(parser, native_parser, False, False)
assert len(lines) == 15
assert not any("--foo" in line for line in lines)
lines = self._format_for_global_scope_with_parser(parser, True, False)
lines = self._format_for_global_scope_with_parser(parser, native_parser, True, False)
assert len(lines) == 24

def test_suppress_deprecated(self) -> None:
Expand Down
36 changes: 35 additions & 1 deletion src/python/pants/help/help_info_extracter.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,11 @@
from pants.engine.rules import Rule, TaskRule
from pants.engine.target import Field, RegisteredTargetTypes, StringField, Target, TargetGenerator
from pants.engine.unions import UnionMembership, UnionRule, is_union
from pants.option.native_options import NativeOptionParser
from pants.option.option_util import is_dict_option, is_list_option
from pants.option.options import Options
from pants.option.parser import OptionValueHistory, Parser
from pants.option.ranked_value import Rank, RankedValue
from pants.option.scope import ScopeInfo
from pants.util.frozendict import LazyFrozenDict
from pants.util.strutil import first_paragraph, strval
Expand Down Expand Up @@ -505,6 +507,7 @@ def load() -> OptionScopeHelpInfo:
return HelpInfoExtracter(scope_info.scope).get_option_scope_help_info(
scope_info.description,
options.get_parser(scope_info.scope),
options.native_parser.with_derivation(),
# `filter` should be treated as a subsystem for `help`, even though it still
# works as a goal for backwards compatibility.
scope_info.is_goal if scope_info.scope != "filter" else False,
Expand Down Expand Up @@ -999,6 +1002,7 @@ def get_option_scope_help_info(
self,
description: str,
parser: Parser,
native_parser: NativeOptionParser,
is_goal: bool,
provider: str = "",
deprecated_scope: Optional[str] = None,
Expand All @@ -1009,7 +1013,37 @@ def get_option_scope_help_info(
advanced_options = []
deprecated_options = []
for args, kwargs in parser.option_registrations_iter():
history = parser.history(kwargs["dest"])
derivation = native_parser.get_derivation(
scope=parser.scope, registration_args=args, registration_kwargs=kwargs
)
# Massage the derivation structure returned by the NativeParser into an
# OptionValueHistory as returned by the legacy parser.
# TODO: Once we get rid of the legacy parser we can probably simplify by
# using the native structure directly.
ranked_values = []

# Adding this constant, empty history entry is silly, but it appears in the
# legacy parser's results as an implementation artifact, and we want to be
# consistent with its tests until we get rid of it.
is_list = kwargs.get("type") == list
is_dict = kwargs.get("type") == dict
empty_val: list | dict | None = [] if is_list else {} if is_dict else None
empty_details = "" if (is_list or is_dict) else None
ranked_values.append(RankedValue(Rank.NONE, empty_val, empty_details))

for value, ranks in derivation:
if len(ranks) == 0:
rank = Rank.NONE
details = None
else:
rank = ranks[-1]
# Again, distinguishing between '' vs None in the details field
# does not matter, but we want to be consistent with the idiosyncratic
# behavior of the legacy parser, until we get rid of it.
details = rank.description() or empty_details
ranked_values.append(RankedValue(rank, value, details))
history = OptionValueHistory(tuple(ranked_values))
# history = parser.history(kwargs["dest"])
ohi = self.get_option_help_info(args, kwargs)
ohi = dataclasses.replace(ohi, value_history=history)
if ohi.deprecation_active:
Expand Down
10 changes: 8 additions & 2 deletions src/python/pants/help/help_info_extracter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from pants.help.help_info_extracter import HelpInfoExtracter, pretty_print_type_hint, to_help_str
from pants.option.config import Config
from pants.option.global_options import GlobalOptions, LogLevelOption
from pants.option.native_options import NativeOptionParser
from pants.option.option_types import BoolOption, IntOption, StrListOption
from pants.option.options import Options
from pants.option.parser import Parser
Expand Down Expand Up @@ -103,9 +104,10 @@ def do_test(args, kwargs, expected_default_str):
config=Config.load([]),
scope_info=GlobalOptions.get_scope_info(),
)
native_parser = NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True)
parser.register(*args, **kwargs)
oshi = HelpInfoExtracter(parser.scope).get_option_scope_help_info(
"description", parser, False, "provider"
"description", parser, native_parser, False, "provider"
)
assert oshi.description == "description"
assert oshi.provider == "provider"
Expand Down Expand Up @@ -202,8 +204,11 @@ def exp_to_len(exp):
config=Config.load([]),
scope_info=GlobalOptions.get_scope_info(),
)
native_parser = NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True)
parser.register("--foo", **kwargs)
oshi = HelpInfoExtracter("").get_option_scope_help_info("", parser, False, "")
oshi = HelpInfoExtracter("").get_option_scope_help_info(
"", parser, native_parser, False, ""
)
assert exp_to_len(expected_basic) == len(oshi.basic)
assert exp_to_len(expected_advanced) == len(oshi.advanced)

Expand Down Expand Up @@ -260,6 +265,7 @@ class BazLibrary(Target):
known_scope_infos=[Global.get_scope_info(), Foo.get_scope_info(), Bar.get_scope_info()],
args=["./pants", "--backend-packages=['internal_plugins.releases']"],
bootstrap_option_values=None,
include_derivation=True,
)
Global.register_options_on_scope(options, UnionMembership({}))
Foo.register_options_on_scope(options, UnionMembership({}))
Expand Down
14 changes: 10 additions & 4 deletions src/python/pants/help/help_tools_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from pants.help.maybe_color import MaybeColor
from pants.option.config import Config
from pants.option.global_options import GlobalOptions
from pants.option.native_options import NativeOptionParser
from pants.option.parser import Parser


Expand All @@ -23,23 +24,28 @@ def parser() -> Parser:
)


@pytest.fixture
def native_parser() -> NativeOptionParser:
return NativeOptionParser([], {}, [], allow_pantsrc=False, include_derivation=True)


@pytest.fixture
def extracter() -> HelpInfoExtracter:
return HelpInfoExtracter("test")


@pytest.fixture
def tool_info(extracter, parser) -> ToolHelpInfo:
def tool_info(extracter, parser, native_parser) -> ToolHelpInfo:
parser.register("version", typ=str, default="1.0")
parser.register("url-template", typ=str, default="https://download/{version}")
oshi = extracter.get_option_scope_help_info("Test description.", parser, False)
oshi = extracter.get_option_scope_help_info("Test description.", parser, native_parser, False)
tool_info = ToolHelpInfo.from_option_scope_help_info(oshi)
assert tool_info is not None
return tool_info


def test_no_tool_help_info(extracter, parser) -> None:
oshi = extracter.get_option_scope_help_info("", parser, False)
def test_no_tool_help_info(extracter, parser, native_parser) -> None:
oshi = extracter.get_option_scope_help_info("", parser, native_parser, False)
assert ToolHelpInfo.from_option_scope_help_info(oshi) is None


Expand Down
Loading

0 comments on commit 34f113a

Please sign in to comment.