From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1658346130; Fri, 24 Jan 2025 19:15:53 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E22EF402CE; Fri, 24 Jan 2025 19:15:52 +0100 (CET) Received: from mail-lj1-f175.google.com (mail-lj1-f175.google.com [209.85.208.175]) by mails.dpdk.org (Postfix) with ESMTP id BC2714028C for ; Fri, 24 Jan 2025 19:15:51 +0100 (CET) Received: by mail-lj1-f175.google.com with SMTP id 38308e7fff4ca-30032baf9d3so1144151fa.2 for ; Fri, 24 Jan 2025 10:15:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1737742551; x=1738347351; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=twHxOZN4Q0z7xt4acwuQ47BvI0Y28ifXUwAfMppJGU8=; b=RXzySFPRtz0yGLnmqZd+bDSsKnlI4C6cPlaitvImcN1P/IKcgKqp8jxc7NuAh/0fWC Y1Ao6y2zTVXyPvD/FP641ZOCS/wE8jCdZ4hy84juHUL7bPvxjto8yXX5whMQMqhgLovP RG16gwfJMXlIPJ6uLDGvoFZYOAyk1iGQz/sc8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737742551; x=1738347351; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=twHxOZN4Q0z7xt4acwuQ47BvI0Y28ifXUwAfMppJGU8=; b=OYX2xNylVCLyl1JkcaAZ67PwFweygdRF7lcfjTISVdSSJAKFvPM3StbxusiEjP4l4Y jijSNqy40M2n4+FAZgrbttWQk5md1RsJ1hfhqNuYLKBEBfT3P2xAY3p8EEc8M7h19Lq2 +DaClufpwqdF4axdl+taW9PVAbLLdSB7twY0Q5SiOjbfiUSUppsBPrQyErJtsIwjN0VM xm/+GD3zeCvddSWDTeNOKP5pOlqxCyCVw19egO3JHA2QaONCvgQPd/nYdi7oy5GFIobN veWZKltk//gpT/cXMtgLf8ZxRMyaS9FCbZ8vPzbXZqMEk7jKKgmtqWR8bAEuPofAZEpu Vs6w== X-Gm-Message-State: AOJu0YzW+UkYockkWD5dEmdFoLNh4SmlSoei0N+y+IrnH8janEYPcoAx KfZ4DiAZ4f5r36aUwYQwkcldOC5XkPBb71Mi7ixYC8Iuw2FZu81HQEihij7sZWs39ZMPXQ5U5oJ d+ke/XU8qC2/wkqfC2oCfBYG1WU9NHv9tYfHJKg== X-Gm-Gg: ASbGncvHakfcSoxl/NQ25IKyNRSqgzGEA3zBnO2jdgqMubsjOPihfGlIyZypJri1hcg w0R4OKoxcqe2B73Z6KW2h42hqBGVClkOj8n5g69E20VXzS4toQ6Wnp5tY8Owcnbw8po7keoEMU4 TONQBUqjFLtus+JZJHeK3T X-Google-Smtp-Source: AGHT+IEZhr9hIKj2grDiAXqdmZiFqWmoCr5EUcl5sYey5YOYfxGQJnhoosDoyS8R7MoFpIE3QrFuAUPloTrhKGMA9CQ= X-Received: by 2002:a05:651c:1505:b0:300:1f36:8fea with SMTP id 38308e7fff4ca-3072cb1f622mr41951331fa.7.1737742551155; Fri, 24 Jan 2025 10:15:51 -0800 (PST) MIME-Version: 1.0 References: <20240613201831.9748-3-npratte@iol.unh.edu> <20250124113909.137128-1-luca.vizzarro@arm.com> <20250124113909.137128-6-luca.vizzarro@arm.com> In-Reply-To: <20250124113909.137128-6-luca.vizzarro@arm.com> From: Nicholas Pratte Date: Fri, 24 Jan 2025 13:15:39 -0500 X-Gm-Features: AWEUYZk5mgSnSrFAj1lVuQ2UQ2yG0n1G8WphEEwmJGm7A3H-PngBmkJrPQSKuS0 Message-ID: Subject: Re: [PATCH v4 5/7] dts: handle CLI overrides in the configuration To: Luca Vizzarro Cc: dev@dpdk.org, Paul Szczepanek , Dean Marx , Patrick Robb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Thank you for addressing this. Great work! Reviewed-by: Nicholas Pratte On Fri, Jan 24, 2025 at 6:39=E2=80=AFAM Luca Vizzarro 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 > Signed-off-by: Luca Vizzarro > Reviewed-by: Paul Szczepanek > Reviewed-by: Dean Marx > --- > 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/__in= it__.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, T= ypedDict, 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:`Validation= Context.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 w= ith the respective names. > + > + Returns: > + Pydantic before model validator. > + """ > + > + def _loader(data: Any, info: ValidationInfo) -> Any: > + if not isinstance(data, MutableMapping): > + return data > + > + settings =3D cast(ValidationContext, info.context)["settings"] > + for field in fields: > + if isinstance(field, tuple): > + settings_field =3D field[0] > + config_field =3D field[1] > + else: > + settings_field =3D config_field =3D field > + > + if settings_data :=3D getattr(settings, settings_field): > + data[config_field] =3D 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 =3D model_validator(mode=3D"before")( > + load_fields_from_settings("dpdk_location") > + ) > + > > class DPDKPrecompiledBuildConfiguration(BaseDPDKBuildConfiguration): > """DPDK precompiled build configuration.""" > @@ -325,6 +372,10 @@ class DPDKPrecompiledBuildConfiguration(BaseDPDKBuil= dConfiguration): > #: subdirectory of `~dpdk_location.dpdk_tree` or `~dpdk_location.tar= ball` root directory. > precompiled_build_dir: str =3D Field(min_length=3D1) > > + build_dir_from_settings =3D model_validator(mode=3D"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 =3D None > > + fields_from_settings =3D model_validator(mode=3D"before")( > + load_fields_from_settings("test_suites", "random_seed") > + ) > + > > class TestRunWithNodesConfiguration(NamedTuple): > """Tuple containing the configuration of the test run and its associ= ated 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) -> Configurat= ion: > > Args: > config_file_path: The path to the YAML test run configuration fi= le. > + 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) -> Configur= ation: > Raises: > ConfigurationError: If the supplied configuration file is invali= d. > """ > - with open(config_file_path, "r") as f: > + with open(settings.config_file_path, "r") as f: > config_data =3D yaml.safe_load(f) > > try: > - return Configuration.model_validate(config_data) > + context =3D ValidationContext(settings=3Dsettings) > + return Configuration.model_validate(config_data, context=3Dconte= xt) > except ValidationError as e: > raise ConfigurationError("failed to load the supplied configurat= ion") 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 an= d string constants.""" > - self._configuration =3D load_config(SETTINGS.config_file_path) > + self._configuration =3D load_config(SETTINGS) > self._logger =3D 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 =3D self._result.add_test_run(test_run_c= onfig) > # we don't want to modify the original config, so create= a copy > - test_run_test_suites =3D list( > - SETTINGS.test_suites if SETTINGS.test_suites else te= st_run_config.test_suites > - ) > + test_run_test_suites =3D test_run_config.test_suites > if not test_run_config.skip_smoke_tests: > test_run_test_suites[:0] =3D [TestSuiteConfig(test_s= uite=3D"smoke_tests")] > try: > @@ -320,15 +317,6 @@ def _run_test_run( > test_run_result.sut_info =3D sut_node.node_info > try: > dpdk_build_config =3D test_run_config.dpdk_config > - if new_location :=3D SETTINGS.dpdk_location: > - dpdk_build_config =3D dpdk_build_config.model_copy( > - update=3D{"dpdk_location": new_location} > - ) > - if dir :=3D SETTINGS.precompiled_build_dir: > - dpdk_build_config =3D DPDKPrecompiledBuildConfiguration( > - dpdk_location=3Ddpdk_build_config.dpdk_location, > - precompiled_build_dir=3Ddir, > - ) > sut_node.set_up_test_run(test_run_config, dpdk_build_config) > test_run_result.dpdk_build_info =3D 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 =3D SETTINGS.random_seed or conf.random_seed or random.rand= range(0xFFFF_FFFF) > + seed =3D 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 >