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 5B5A5A0032; Tue, 13 Sep 2022 19:47:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F41024021D; Tue, 13 Sep 2022 19:47:47 +0200 (CEST) Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) by mails.dpdk.org (Postfix) with ESMTP id D5A3B40151 for ; Tue, 13 Sep 2022 19:47:46 +0200 (CEST) Received: by mail-pl1-f171.google.com with SMTP id p18so12572331plr.8 for ; Tue, 13 Sep 2022 10:47:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=MgdSCkYMQVqFUXsv4FFArRBonknyK9b/hJDPf8ZFt1I=; b=bl8k44+IvMZfaLdiUIM3Tl37EkIYBjFY+cx2rUTSE73EyfijLTuhirw2ygT1EBLDDM Nh5v3uWdM5SEv+dKhGT5v/VAtXWHyYcBWqShxD+toCAINMX/ONeSknsowV5yOtnBiIwl sg6Nb8wkMUYZadZTY4owxDE7N7TRBFieda0ic= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=MgdSCkYMQVqFUXsv4FFArRBonknyK9b/hJDPf8ZFt1I=; b=hCqQr9MhFv2nazx71NEeZN/dSVSjjvcjNEmWxPtoE80kN/V6EghxPcwIMMyVNgi8jj MUUCPpXEQxXqDpe7eAeRf7Cfr/nwcvZ651pa+EvdPVHURUQsfI31RHU/D0+qFXkPaLJV v03X4M+5chfWdwSBN71I0Vxjp+4PzwEn2HRm2mJ4d3oOOqxKcP2XJ8zrlWc+9I4NeU8z uB/dw8+VR+sNRhoWiS4yFqSV10z1lUL7aLA5xJ1IUZGfRUfnxUHs2VHeilCLaxAVVsSa QdUJFLFk6QhO8Dmr7E7g9kcsFa8fnZAM7d6x2aeITDzc2lv26GiDVNffuZRjN5Cww2mN wSug== X-Gm-Message-State: ACrzQf2SZfwVa1T95O5Vu3CBpdobuJPLKnJx1J+KF/T97u73kLJq+/ns IujXfIYUaw4cPG1iP+/jFPUQIfzxYa6mTQu5ETyAsA== X-Google-Smtp-Source: AMsMyM7IKfFcCSfu85DxkjUcVu0dBxjuKr7/ZPdBdR5QafI5C33135wLg2gU+mB1XYrGFI2Nr0xM2Ky4zTZ7CV7R6+g= X-Received: by 2002:a17:90b:17cf:b0:202:95a2:e310 with SMTP id me15-20020a17090b17cf00b0020295a2e310mr439146pjb.76.1663091265915; Tue, 13 Sep 2022 10:47:45 -0700 (PDT) MIME-Version: 1.0 References: <20220728100044.1318484-1-juraj.linkes@pantheon.tech> <20220729105550.1382664-1-juraj.linkes@pantheon.tech> <20220729105550.1382664-7-juraj.linkes@pantheon.tech> In-Reply-To: From: Owen Hilyard Date: Tue, 13 Sep 2022 13:47:10 -0400 Message-ID: Subject: Re: [PATCH v4 6/9] dts: add config parser module To: Bruce Richardson Cc: =?UTF-8?Q?Juraj_Linke=C5=A1?= , Thomas Monjalon , David Marchand , ronan.randles@intel.com, Honnappa Nagarahalli , "Tu, Lijuan" , dev Content-Type: multipart/alternative; boundary="00000000000022692505e8929c28" 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 --00000000000022692505e8929c28 Content-Type: text/plain; charset="UTF-8" > > +# Frozen makes the object immutable. This enables further optimizations, > > +# and makes it thread safe should we every want to move in that > direction. > > +@dataclass(slots=True, frozen=True) > > +class NodeConfiguration: > > + name: str > > + hostname: str > > + user: str > > + password: Optional[str] > > + > > + @staticmethod > > + def from_dict(d: dict) -> "NodeConfiguration": > > + return NodeConfiguration( > > + name=d["name"], > > + hostname=d["hostname"], > > + user=d["user"], > > + password=d.get("password"), > > + ) > > + > Out of curiosity, what is the reason for having a static "from_dict" method > rather than just a regular constructor function that takes a dict as > parameter? > @dataclass(...) is a class annotation that transforms the thing it annotates into a dataclass. This means it creates the constructor for you based on the property type annotations. If you create your own constructor, you need a constructor that can either take a single dictionary or all of the parameters like a normal constructor. Making it a static method also means that each class can manage how it should be constructed from a dictionary. Some of the other classes will transform lists or perform other assertions. It also makes it easier to have specialized types. For instance, a NICConfiguration class would have to handle all of the possible device arguments that could be passed to any PMD driver if things were passed as parameters. > > + > > +@dataclass(slots=True, frozen=True) > > +class ExecutionConfiguration: > > + system_under_test: NodeConfiguration > > + > Minor comment: seems strange having only a single member variable in this > class, effectively duplicating the class above. > More is intended to go here. For instance, what tests to run, configuration for virtual machines, the traffic generator node. > > + @staticmethod > > + def from_dict(d: dict, node_map: dict) -> "ExecutionConfiguration": > from reading the code it appears that node_map is a dict of > NodeConfiguration objects, right? Might be worth adding that to the > definition for clarity, and also the specific type of the dict "d" (if it > has one) > > + sut_name = d["system_under_test"] > > + assert sut_name in node_map, f"Unknown SUT {sut_name} in > execution {d}" > > + > > + return ExecutionConfiguration( > > + system_under_test=node_map[sut_name], > > + ) > > + > > + > > +@dataclass(slots=True, frozen=True) > > +class Configuration: > > + executions: list[ExecutionConfiguration] > > + > > + @staticmethod > > + def from_dict(d: dict) -> "Configuration": > > + nodes: list[NodeConfiguration] = list( > > + map(NodeConfiguration.from_dict, d["nodes"]) > So "d" is a dict of dicts? > d is a dictionary which matches the json schema for the class. In the case of the Configuration class, it is a dictionary matching the entire json schema. > > + ) > > + assert len(nodes) > 0, "There must be a node to test" > > + > > + node_map = {node.name: node for node in nodes} > > + assert len(nodes) == len(node_map), "Duplicate node names are > not allowed" > > + > > + executions: list[ExecutionConfiguration] = list( > > + map( > > + ExecutionConfiguration.from_dict, d["executions"], > [node_map for _ in d] > > + ) > > + ) > > + > > + return Configuration(executions=executions) > > + > > + > > +def load_config() -> Configuration: > > + """ > > + Loads the configuration file and the configuration file schema, > > + validates the configuration file, and creates a configuration > object. > > + """ > > + with open(SETTINGS.config_file_path, "r") as f: > > + config_data = yaml.safe_load(f) > > + > > + schema_path = os.path.join( > > + pathlib.Path(__file__).parent.resolve(), "conf_yaml_schema.json" > > + ) > > + > > + with open(schema_path, "r") as f: > > + schema = json.load(f) > > + config: dict[str, Any] = warlock.model_factory(schema, > name="_Config")(config_data) > > + config_obj: Configuration = Configuration.from_dict(dict(config)) > > + return config_obj > > + > > + > > +CONFIGURATION = load_config() > --00000000000022692505e8929c28 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
<snip>=C2=A0
> +# Frozen makes the object immutable. This enables further optimizatio= ns,
> +# and makes it thread safe should we every want to move in that direc= tion.
> +@dataclass(slots=3DTrue, frozen=3DTrue)
> +class NodeConfiguration:
> +=C2=A0 =C2=A0 name: str
> +=C2=A0 =C2=A0 hostname: str
> +=C2=A0 =C2=A0 user: str
> +=C2=A0 =C2=A0 password: Optional[str]
> +
> +=C2=A0 =C2=A0 @staticmethod
> +=C2=A0 =C2=A0 def from_dict(d: dict) -> "NodeConfiguration&qu= ot;:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return NodeConfiguration(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 name=3Dd["name"],=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hostname=3Dd["hostname= "],
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 user=3Dd["user"],=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 password=3Dd.get("pass= word"),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
Out of curiosity, what is the reason for having a static "from_dict&qu= ot; method
rather than just a regular constructor function that takes a dict as
parameter?

@dataclass(...) is a class a= nnotation that transforms the thing it annotates into a dataclass. This mea= ns it creates the constructor for you based on the property type annotation= s. If you create your own constructor, you need a constructor that can eith= er take a single dictionary or all of the parameters like a normal construc= tor. Making it a static method also means that each class can manage how it= should be constructed from a dictionary. Some of the other classes will tr= ansform lists or perform other assertions. It also makes it easier to have = specialized types. For instance, a NICConfiguration class would have to han= dle all of the possible device arguments that could be passed to any PMD dr= iver if things were passed as parameters.=C2=A0
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex"> > +
> +@dataclass(slots=3DTrue, frozen=3DTrue)
> +class ExecutionConfiguration:
> +=C2=A0 =C2=A0 system_under_test: NodeConfiguration
> +
Minor comment: seems strange having only a single member variable in this class, effectively duplicating the class above.

More is intended to go here. For instance, what tests to run, conf= iguration for virtual machines, the traffic generator node.
=C2= =A0
<snip>
> +=C2=A0 =C2=A0 @staticmethod
> +=C2=A0 =C2=A0 def from_dict(d: dict, node_map: dict) -> "Exec= utionConfiguration":
from reading the code it appears that node_map is a dict of
NodeConfiguration objects, right? Might be worth adding that to the
definition for clarity, and also the specific type of the dict "d"= ; (if it
has one)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_name =3D d["system_under_test&qu= ot;]
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert sut_name in node_map, f"Unkno= wn SUT {sut_name} in execution {d}"
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return ExecutionConfiguration(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 system_under_test=3Dnode_ma= p[sut_name],
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +
> +@dataclass(slots=3DTrue, frozen=3DTrue)
> +class Configuration:
> +=C2=A0 =C2=A0 executions: list[ExecutionConfiguration]
> +
> +=C2=A0 =C2=A0 @staticmethod
> +=C2=A0 =C2=A0 def from_dict(d: dict) -> "Configuration":=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 nodes: list[NodeConfiguration] =3D list(<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 map(NodeConfiguration.from_= dict, d["nodes"])
So "d" is a dict of dicts?

d = is a dictionary which matches the json schema for the class. In the case of= the Configuration class, it is a dictionary matching the entire json schem= a.
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert len(nodes) > 0, "There mus= t be a node to test"
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 node_map =3D {node.name: node for node in node= s}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert len(nodes) =3D=3D len(node_map), &= quot;Duplicate node names are not allowed"
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 executions: list[ExecutionConfiguration] = =3D list(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 map(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ExecutionConf= iguration.from_dict, d["executions"], [node_map for _ in d]
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return Configuration(executions=3Dexecuti= ons)
> +
> +
> +def load_config() -> Configuration:
> +=C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 Loads the configuration file and the configuration file= schema,
> +=C2=A0 =C2=A0 validates the configuration file, and creates a configu= ration object.
> +=C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 with open(SETTINGS.config_file_path, "r") as = f:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 config_data =3D yaml.safe_load(f)
> +
> +=C2=A0 =C2=A0 schema_path =3D os.path.join(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 pathlib.Path(__file__).parent.resolve(), = "conf_yaml_schema.json"
> +=C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 with open(schema_path, "r") as f:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 schema =3D json.load(f)
> +=C2=A0 =C2=A0 config: dict[str, Any] =3D warlock.model_factory(schema= , name=3D"_Config")(config_data)
> +=C2=A0 =C2=A0 config_obj: Configuration =3D Configuration.from_dict(d= ict(config))
> +=C2=A0 =C2=A0 return config_obj
> +
> +
> +CONFIGURATION =3D load_config()
<snip>
--00000000000022692505e8929c28--