Re: [PATCH v4 5/7] dts: handle CLI overrides in the configuration

[Nicholas Pratte <npratte@iol.unh.edu>]

Thank you for addressing this. Great work!

Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>

On Fri, Jan 24, 2025 at 6:39 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The current handling of the configuration loading is inconsistent. After
> the whole configuration is loaded, if there are any CLI or environment
> overrides set, the code forcefully modifies the frozen configuration to
> use them.
>
> This changes the handling by passing the environment/CLI settings as
> part of the configuration context and handle the overrides directly at
> the field level before these are validated. As a positive side effect,
> the validator won't complain if a field is missing from the file but it
> has been specified as an environment/CLI override.
>
> Bugzilla ID: 1360
> Bugzilla ID: 1598
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Dean Marx <dmarx@iol.unh.edu>
> ---
>  dts/framework/config/__init__.py | 65 ++++++++++++++++++++++++++++++--
>  dts/framework/runner.py          | 18 ++-------
>  2 files changed, 64 insertions(+), 19 deletions(-)
>
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index 2496f48e20..6ae98d0387 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -33,10 +33,11 @@
>  """
>
>  import tarfile
> +from collections.abc import Callable, MutableMapping
>  from enum import Enum, auto, unique
>  from functools import cached_property
>  from pathlib import Path, PurePath
> -from typing import TYPE_CHECKING, Annotated, Any, Literal, NamedTuple
> +from typing import TYPE_CHECKING, Annotated, Any, Literal, NamedTuple, TypedDict, cast
>
>  import yaml
>  from pydantic import (
> @@ -44,18 +45,60 @@
>      ConfigDict,
>      Field,
>      ValidationError,
> +    ValidationInfo,
>      field_validator,
>      model_validator,
>  )
>  from typing_extensions import Self
>
>  from framework.exception import ConfigurationError
> +from framework.settings import Settings
>  from framework.utils import REGEX_FOR_PCI_ADDRESS, StrEnum
>
>  if TYPE_CHECKING:
>      from framework.test_suite import TestSuiteSpec
>
>
> +class ValidationContext(TypedDict):
> +    """A context dictionary to use for validation."""
> +
> +    #: The command line settings.
> +    settings: Settings
> +
> +
> +def load_fields_from_settings(
> +    *fields: str | tuple[str, str],
> +) -> Callable[[Any, ValidationInfo], Any]:
> +    """Before model validator that injects values from :attr:`ValidationContext.settings`.
> +
> +    Args:
> +        *fields: The name of the fields to apply the argument value to. If the settings field name
> +            is not the same as the configuration field, supply a tuple with the respective names.
> +
> +    Returns:
> +        Pydantic before model validator.
> +    """
> +
> +    def _loader(data: Any, info: ValidationInfo) -> Any:
> +        if not isinstance(data, MutableMapping):
> +            return data
> +
> +        settings = cast(ValidationContext, info.context)["settings"]
> +        for field in fields:
> +            if isinstance(field, tuple):
> +                settings_field = field[0]
> +                config_field = field[1]
> +            else:
> +                settings_field = config_field = field
> +
> +            if settings_data := getattr(settings, settings_field):
> +                data[config_field] = settings_data
> +
> +        return data
> +
> +    return _loader
> +
> +
>  class FrozenModel(BaseModel):
>      """A pre-configured :class:`~pydantic.BaseModel`."""
>
> @@ -317,6 +360,10 @@ class BaseDPDKBuildConfiguration(FrozenModel):
>      #: The location of the DPDK tree.
>      dpdk_location: DPDKLocation
>
> +    dpdk_location_from_settings = model_validator(mode="before")(
> +        load_fields_from_settings("dpdk_location")
> +    )
> +
>
>  class DPDKPrecompiledBuildConfiguration(BaseDPDKBuildConfiguration):
>      """DPDK precompiled build configuration."""
> @@ -325,6 +372,10 @@ class DPDKPrecompiledBuildConfiguration(BaseDPDKBuildConfiguration):
>      #: subdirectory of `~dpdk_location.dpdk_tree` or `~dpdk_location.tarball` root directory.
>      precompiled_build_dir: str = Field(min_length=1)
>
> +    build_dir_from_settings = model_validator(mode="before")(
> +        load_fields_from_settings("precompiled_build_dir")
> +    )
> +
>
>  class DPDKBuildOptionsConfiguration(FrozenModel):
>      """DPDK build options configuration.
> @@ -439,6 +490,10 @@ class TestRunConfiguration(FrozenModel):
>      #: The seed to use for pseudo-random generation.
>      random_seed: int | None = None
>
> +    fields_from_settings = model_validator(mode="before")(
> +        load_fields_from_settings("test_suites", "random_seed")
> +    )
> +
>
>  class TestRunWithNodesConfiguration(NamedTuple):
>      """Tuple containing the configuration of the test run and its associated nodes."""
> @@ -541,7 +596,7 @@ def validate_test_runs_with_nodes(self) -> Self:
>          return self
>
>
> -def load_config(config_file_path: Path) -> Configuration:
> +def load_config(settings: Settings) -> Configuration:
>      """Load DTS test run configuration from a file.
>
>      Load the YAML test run configuration file, validate it, and create a test run configuration
> @@ -552,6 +607,7 @@ def load_config(config_file_path: Path) -> Configuration:
>
>      Args:
>          config_file_path: The path to the YAML test run configuration file.
> +        settings: The settings provided by the user on the command line.
>
>      Returns:
>          The parsed test run configuration.
> @@ -559,10 +615,11 @@ def load_config(config_file_path: Path) -> Configuration:
>      Raises:
>          ConfigurationError: If the supplied configuration file is invalid.
>      """
> -    with open(config_file_path, "r") as f:
> +    with open(settings.config_file_path, "r") as f:
>          config_data = yaml.safe_load(f)
>
>      try:
> -        return Configuration.model_validate(config_data)
> +        context = ValidationContext(settings=settings)
> +        return Configuration.model_validate(config_data, context=context)
>      except ValidationError as e:
>          raise ConfigurationError("failed to load the supplied configuration") from e
> diff --git a/dts/framework/runner.py b/dts/framework/runner.py
> index 0cdbb07e06..e46a8c1a4f 100644
> --- a/dts/framework/runner.py
> +++ b/dts/framework/runner.py
> @@ -31,7 +31,6 @@
>
>  from .config import (
>      Configuration,
> -    DPDKPrecompiledBuildConfiguration,
>      SutNodeConfiguration,
>      TestRunConfiguration,
>      TestSuiteConfig,
> @@ -82,7 +81,7 @@ class DTSRunner:
>
>      def __init__(self):
>          """Initialize the instance with configuration, logger, result and string constants."""
> -        self._configuration = load_config(SETTINGS.config_file_path)
> +        self._configuration = load_config(SETTINGS)
>          self._logger = get_dts_logger()
>          if not os.path.exists(SETTINGS.output_dir):
>              os.makedirs(SETTINGS.output_dir)
> @@ -142,9 +141,7 @@ def run(self) -> None:
>                  self._init_random_seed(test_run_config)
>                  test_run_result = self._result.add_test_run(test_run_config)
>                  # we don't want to modify the original config, so create a copy
> -                test_run_test_suites = list(
> -                    SETTINGS.test_suites if SETTINGS.test_suites else test_run_config.test_suites
> -                )
> +                test_run_test_suites = test_run_config.test_suites
>                  if not test_run_config.skip_smoke_tests:
>                      test_run_test_suites[:0] = [TestSuiteConfig(test_suite="smoke_tests")]
>                  try:
> @@ -320,15 +317,6 @@ def _run_test_run(
>          test_run_result.sut_info = sut_node.node_info
>          try:
>              dpdk_build_config = test_run_config.dpdk_config
> -            if new_location := SETTINGS.dpdk_location:
> -                dpdk_build_config = dpdk_build_config.model_copy(
> -                    update={"dpdk_location": new_location}
> -                )
> -            if dir := SETTINGS.precompiled_build_dir:
> -                dpdk_build_config = DPDKPrecompiledBuildConfiguration(
> -                    dpdk_location=dpdk_build_config.dpdk_location,
> -                    precompiled_build_dir=dir,
> -                )
>              sut_node.set_up_test_run(test_run_config, dpdk_build_config)
>              test_run_result.dpdk_build_info = sut_node.get_dpdk_build_info()
>              tg_node.set_up_test_run(test_run_config, dpdk_build_config)
> @@ -622,6 +610,6 @@ def _exit_dts(self) -> None:
>
>      def _init_random_seed(self, conf: TestRunConfiguration) -> None:
>          """Initialize the random seed to use for the test run."""
> -        seed = SETTINGS.random_seed or conf.random_seed or random.randrange(0xFFFF_FFFF)
> +        seed = conf.random_seed or random.randrange(0xFFFF_FFFF)
>          self._logger.info(f"Initializing test run with random seed {seed}.")
>          random.seed(seed)
> --
> 2.43.0
>