Thank you for addressing this. Great work!
Reviewed-by: Nicholas Pratte <npra...@iol.unh.edu>
On Fri, Jan 24, 2025 at 6:39 AM Luca Vizzarro <luca.vizza...@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 <npra...@iol.unh.edu>
> Signed-off-by: Luca Vizzarro <luca.vizza...@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepa...@arm.com>
> Reviewed-by: Dean Marx <dm...@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
>
