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 C44BC43E2A; Tue, 9 Apr 2024 14:10:21 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 81B00402C6; Tue, 9 Apr 2024 14:10:21 +0200 (CEST) Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by mails.dpdk.org (Postfix) with ESMTP id 7CFA7402C0 for ; Tue, 9 Apr 2024 14:10:19 +0200 (CEST) Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-a51addddbd4so427753866b.0 for ; Tue, 09 Apr 2024 05:10:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712664619; x=1713269419; 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=qTmspKwsvbvriEOfhSyaMk9ptoZBqAcuV8DTsWxXOAo=; b=uPo7T9U0SWYaMfWDu0GmOy1pEEdbzyhuzGkKr/HNZt5O5xIzLu9pntmXrCgJLv9CH3 eqdEQgrmcCR6i1sTb4VfppBkU0YM7qTr8W5Xr1xD1QztpnmJ5sEYQMOyoIEORr+SAMEi J+1ObXZcZ7DyrWX8BzPWuFGq3yRH05THfsLjyHbFh51uHEx2XxyXzMkqa/FN3qSnqx/e IzsyeLj8g1sN7v68V+8qoaJ08po5pzsSn5YidP90XUBtPqedEOEqX7uqxIZL1rRKnbXL RuFPgsfmM1E+Y3kiDMvI9VQxgiJm6GR49JqxvrKFshO/196oF+nmBYhdqJLopIjCTUx1 7TVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712664619; x=1713269419; 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=qTmspKwsvbvriEOfhSyaMk9ptoZBqAcuV8DTsWxXOAo=; b=bSGThP0ZyzL95Ao6/5T0uu36I2kmUPxK/tmfn8Zjhb0V5v5laIR9I8JtoVa9JazrYq 8NO6iIhUYUOUMgBgZJjFAFi7nTYscWHa+uN4QPgIvXHqcY53neDXWv8xgQHyQd9LNvBX 7FaiPV3E8Onym4IwipzNf/kqd45qHvd9aYrFZxPzP1BWTD8oLwe3IJrUzSYkox5sbq6K WW9er8+OjDRIL6fat6uANxOcikZw/Xt4fNh69txNie1B7R4UEIWT9u/ihOclpIuXQ1Ky Js7akzDOefPNS4Rhxhb+Lik8fXCtBmcqMrWkl6GI8wh1jnvibKBVcXZmfDUbZIDOQzAa c0hQ== X-Gm-Message-State: AOJu0Yz+uRZZEHh1br3P7svTkvaZveB3ba2Ym/h/oU33dypNfEEqf/OT P13MxvPq7aFqSgfCe29vaPZ69Dy2w9bIjXY2sSEj61NbV6EwmYbT05DkcKzSqWQVKLl/iw2S93a Z5QEu1XPowglkcSEBuTJdNXpHQKtQ+355YVOiQg== X-Google-Smtp-Source: AGHT+IH6TEgKv30mnungwVkcbhRy+q9g07wBFWJRhu5KGl58esau0GRBTNpxnA+HCkiiDiUtb0MxG6FjK9UA1L42xBg= X-Received: by 2002:a17:907:d92:b0:a51:c1e0:2ba9 with SMTP id go18-20020a1709070d9200b00a51c1e02ba9mr7020383ejc.11.1712664618981; Tue, 09 Apr 2024 05:10:18 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-2-luca.vizzarro@arm.com> In-Reply-To: <20240326190422.577028-2-luca.vizzarro@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 9 Apr 2024 14:10:08 +0200 Message-ID: Subject: Re: [PATCH 1/6] dts: add parameters data structure To: Luca Vizzarro Cc: dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli 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 On Tue, Mar 26, 2024 at 8:04=E2=80=AFPM Luca Vizzarro wrote: > > This commit introduces a new "params" module, which adds a new way > to manage command line parameters. The provided Params dataclass > is able to read the fields of its child class and produce a string > representation to supply to the command line. Any data structure > that is intended to represent command line parameters can inherit > it. The representation can then manipulated by using the dataclass > field metadata in conjunction with the provided functions: > > * value_only, used to supply a value without forming an option/flag > * options_end, used to prefix with an options end delimiter (`--`) > * short, used to define a short parameter name, e.g. `-p` > * long, used to define a long parameter name, e.g. `--parameter` > * multiple, used to turn a list into repeating parameters > * field_mixins, used to manipulate the string representation of the > value We shouldn't list what the patch does, but rather explain the choices made within. It seems to me the patch is here to give devs an API instead of them having to construct strings - explaining why this approach improves the old one should be in the commit message. > > Signed-off-by: Luca Vizzarro > Reviewed-by: Jack Bond-Preston > Reviewed-by: Honnappa Nagarahalli > --- > dts/framework/params.py | 232 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 232 insertions(+) > create mode 100644 dts/framework/params.py > > diff --git a/dts/framework/params.py b/dts/framework/params.py > new file mode 100644 > index 0000000000..6b48c8353e > --- /dev/null > +++ b/dts/framework/params.py > @@ -0,0 +1,232 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2024 Arm Limited > + > +"""Parameter manipulation module. > + > +This module provides :class:`~Params` which can be used to model any dat= a structure > +that is meant to represent any command parameters. > +""" > + > +from dataclasses import dataclass, field, fields > +from typing import Any, Callable, Literal, Reversible, TypeVar, Iterable > +from enum import Flag > + > + > +T =3D TypeVar("T") > +#: Type for a Mixin. > +Mixin =3D Callable[[Any], str] > +#: Type for an option parameter. > +Option =3D Literal[True, None] > +#: Type for a yes/no option parameter. > +BooleanOption =3D Literal[True, False, None] > + > +META_VALUE_ONLY =3D "value_only" > +META_OPTIONS_END =3D "options_end" > +META_SHORT_NAME =3D "short_name" > +META_LONG_NAME =3D "long_name" > +META_MULTIPLE =3D "multiple" > +META_MIXINS =3D "mixins" These are only used in the Params class (but not set), so I'd either move them there or make them private. > + > + > +def value_only(metadata: dict[str, Any] =3D {}) -> dict[str, Any]: > + """Injects the value of the attribute as-is without flag. Metadata m= odifier for :func:`dataclasses.field`.""" > + return {**metadata, META_VALUE_ONLY: True} These methods, on the other hand, are used outside this module, so it makes sense to have them outside Params. It could be better to have them inside as static methods, as they're closely related. Looking at how they're used, we should unite the imports: 1. In testpmd_shell, they're imported directly (from framework.params import long) 2. In sut_node, just the params module is imported (from framework import params and then accessed via it: metadata=3Dparams.short("l")) If we move these to Params, we could import Params and use them similarly to 2. Not sure which one is better. > + > + > +def short(name: str, metadata: dict[str, Any] =3D {}) -> dict[str, Any]: It seems to me that having the metadata argument doesn't do anything in these basic functions. > + """Overrides any parameter name with the given short option. Metadat= a modifier for :func:`dataclasses.field`. > + > + .. code:: python > + > + logical_cores: str | None =3D field(default=3D"1-4", metadata=3D= short("l")) > + > + will render as ``-l=3D1-4`` instead of ``--logical-cores=3D1-4``. > + """ > + return {**metadata, META_SHORT_NAME: name} > + > + > +def long(name: str, metadata: dict[str, Any] =3D {}) -> dict[str, Any]: > + """Overrides the inferred parameter name to the specified one. Metad= ata modifier for :func:`dataclasses.field`. > + > + .. code:: python > + > + x_name: str | None =3D field(default=3D"y", metadata=3Dlong("x")= ) > + > + will render as ``--x=3Dy``, but the field is accessed and modified t= hrough ``x_name``. > + """ > + return {**metadata, META_LONG_NAME: name} > + > + > +def options_end(metadata: dict[str, Any] =3D {}) -> dict[str, Any]: > + """Precedes the value with an options end delimiter (``--``). Metada= ta modifier for :func:`dataclasses.field`.""" > + return {**metadata, META_OPTIONS_END: True} > + > + > +def multiple(metadata: dict[str, Any] =3D {}) -> dict[str, Any]: > + """Specifies that this parameter is set multiple times. Must be a li= st. Metadata modifier for :func:`dataclasses.field`. > + > + .. code:: python > + > + ports: list[int] | None =3D field(default_factory=3Dlambda: [0, = 1, 2], metadata=3Dmultiple(param_name("port"))) > + > + will render as ``--port=3D0 --port=3D1 --port=3D2``. Note that modif= iers can be chained like in this example. > + """ > + return {**metadata, META_MULTIPLE: True} > + > + > +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] =3D {}) -> dic= t[str, Any]: Any reason why mixins are plural? I've only seen this used with one argument, do you anticipate we'd need to use more than one? We could make this singular and if we ever need to do two things, we could just pass a function that does those two things (or calls two different functions). Also, I'd just rename the mixin the func or something like that. The default of an argument should not be mutable, here's a quick explanation: https://docs.python-guide.org/writing/gotchas/#mutable-default= -arguments I don't really like the name. The positional arguments are applied to the value and that should be reflected in the name - I'd like to see something like convert in the name. > + """Takes in a variable number of mixins to manipulate the value's re= ndering. Metadata modifier for :func:`dataclasses.field`. > + > + The ``metadata`` keyword argument can be used to chain metadata modi= fiers together. > + > + Mixins can be chained together, executed from right to left in the a= rguments list order. > + > + Example: > + > + .. code:: python > + > + hex_bitmask: int | None =3D field(default=3D0b1101, metadata=3Df= ield_mixins(hex, metadata=3Dparam_name("mask"))) > + > + will render as ``--mask=3D0xd``. The :func:`hex` built-in can be use= d as a mixin turning a valid integer into a hexadecimal representation. > + """ > + return {**metadata, META_MIXINS: mixins} metadata | {META_MIXINS: mixins} also creates a new dictionary with values from both and I think that would be more readable (since it's mentioned in docs: https://docs.python.org/3/library/stdtypes.html#mapping-types-dict). > + > + > +def _reduce_mixins(mixins: Reversible[Mixin], value: Any) -> str: > + for mixin in reversed(mixins): > + value =3D mixin(value) > + return value > + > + > +def str_mixins(*mixins: Mixin): Again the name, this doesn't strike me as a decorator name. A decorator modifies the thing it's decorating so it should contain a verb describing what it's doing. Maybe we could also do singular mixin here, as I described above. It may result in cleaner API. > + """Decorator which modifies the ``__str__`` method, enabling support= for mixins. > + > + Mixins can be chained together, executed from right to left in the a= rguments list order. > + > + Example: > + > + .. code:: python > + > + @str_mixins(hex_from_flag_value) > + class BitMask(enum.Flag): > + A =3D auto() > + B =3D auto() > + > + will allow ``BitMask`` to render as a hexadecimal value. > + """ > + > + def _class_decorator(original_class): > + original_class.__str__ =3D lambda self: _reduce_mixins(mixins, s= elf) > + return original_class > + > + return _class_decorator > + > + > +def comma_separated(values: Iterable[T]) -> str: > + """Mixin which renders an iterable in a comma-separated string.""" > + return ",".join([str(value).strip() for value in values if value is = not None]) > + > + > +def bracketed(value: str) -> str: > + """Mixin which adds round brackets to the input.""" > + return f"({value})" > + > + > +def str_from_flag_value(flag: Flag) -> str: > + """Mixin which returns the value from a :class:`enum.Flag` as a stri= ng.""" > + return str(flag.value) > + > + > +def hex_from_flag_value(flag: Flag) -> str: > + """Mixin which turns a :class:`enum.Flag` value into hexadecimal.""" > + return hex(flag.value) > + > + > +def _make_option(param_name: str, short: bool =3D False, no: bool =3D Fa= lse) -> str: > + param_name =3D param_name.replace("_", "-") > + return f"{'-' if short else '--'}{'no-' if no else ''}{param_name}" > + > + > +@dataclass > +class Params: > + """Helper abstract dataclass that renders its fields into command li= ne arguments. Let's make the abstract class explicitly abstract with https://docs.python.org/3/library/abc.html#abc.ABC. It won't be a full abstract class since it won't have any abstract method or properties, but I think it'll be better this way. > + > + The parameter name is taken from the field name by default. The foll= owing: > + > + .. code:: python > + > + name: str | None =3D "value" > + > + is rendered as ``--name=3Dvalue``. > + Through :func:`dataclasses.field` the resulting parameter can be man= ipulated by applying > + appropriate metadata. This class can be used with the following meta= data modifiers: > + > + * :func:`value_only` > + * :func:`options_end` > + * :func:`short` > + * :func:`long` > + * :func:`multiple` > + * :func:`field_mixins` > + > + To use fields as option switches set the value to ``True`` to enable= them. If you There should be a comma between switches and set. > + use a yes/no option switch you can also set ``False`` which would en= able an option > + prefixed with ``--no-``. Examples: > + > + .. code:: python > + > + interactive: Option =3D True # renders --interactive > + numa: BooleanOption =3D False # renders --no-numa I'm wondering whether these could be simplified, but it's pretty good this way. I'd change the names though: Option -> Switch BooleanOption -> NoSwitch (or YesNoSwitch? NegativeSwitch? Not sure about this one) All options (short, long, etc.) are options so that's where it's confusing. The BooleanOption doesn't really capture what we're doing with it (prepending no-) so I want a name that captures it. There could also be some confusion with this being different from the rest of the options API. This only requires us to set the type, the rest must be passed in dataclasses.field. It's probably not that big of a deal though. > + > + Setting ``None`` will disable any option. I'd reword this to "Setting an option to ``None`` will prevent it from being rendered." or something similar. Disabling an option is kinda ambiguous. The :attr:`~Option` type alias is provided for > + regular option switches, whereas :attr:`~BooleanOption` is offered f= or yes/no ones. > + > + An instance of a dataclass inheriting ``Params`` can also be assigne= d to an attribute, this helps with grouping parameters > + together. The attribute holding the dataclass will be ignored and th= e latter will just be rendered as expected. > + """ > + > + def __str__(self) -> str: > + arguments: list[str] =3D [] > + > + for field in fields(self): > + value =3D getattr(self, field.name) > + > + if value is None: > + continue > + > + options_end =3D field.metadata.get(META_OPTIONS_END, False) > + if options_end: > + arguments.append("--") This is a confusing way to separate the options. The separator itself is an option (I'd rename it to sep or separator instead of end), so I'd make a separate field for it. We could pass init=3DFalse to field() in the field definition, but that relies on fields being ordered, so we'd need to also pass order=3DTrue to the dataclass constructor. > + > + value_only =3D field.metadata.get(META_VALUE_ONLY, False) > + if isinstance(value, Params) or value_only or options_end: > + arguments.append(str(value)) > + continue > + > + # take "short_name" metadata, or "long_name" metadata, or in= fer from field name > + option_name =3D field.metadata.get( > + META_SHORT_NAME, field.metadata.get(META_LONG_NAME, fiel= d.name) > + ) > + is_short =3D META_SHORT_NAME in field.metadata > + > + if isinstance(value, bool): > + arguments.append(_make_option(option_name, short=3Dis_sh= ort, no=3D(not value))) > + continue > + > + option =3D _make_option(option_name, short=3Dis_short) > + separator =3D " " if is_short else "=3D" > + str_mixins =3D field.metadata.get(META_MIXINS, []) > + multiple =3D field.metadata.get(META_MULTIPLE, False) > + > + values =3D value if multiple else [value] > + for entry_value in values: > + entry_value =3D _reduce_mixins(str_mixins, entry_value) > + arguments.append(f"{option}{separator}{entry_value}") > + > + return " ".join(arguments) > + > + > +@dataclass > +class StrParams(Params): > + """A drop-in replacement for parameters passed as a string.""" > + > + value: str =3D field(metadata=3Dvalue_only()) > -- > 2.34.1 >