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 5890F43252; Tue, 31 Oct 2023 13:10:30 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BA5354026C; Tue, 31 Oct 2023 13:10:29 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 5BB87400EF for ; Tue, 31 Oct 2023 13:10:28 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4A9DB1474; Tue, 31 Oct 2023 05:11:09 -0700 (PDT) Received: from [10.1.32.131] (e125442.arm.com [10.1.32.131]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 93F383F738; Tue, 31 Oct 2023 05:10:25 -0700 (PDT) Message-ID: <2f79eb4a-eda0-43a5-a7f6-1c7af84a47e6@foss.arm.com> Date: Tue, 31 Oct 2023 12:10:15 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH v4 4/4] dts: format docstrigs to google format To: =?UTF-8?Q?Juraj_Linke=C5=A1?= , thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, lijuan.tu@intel.com, bruce.richardson@intel.com, jspewock@iol.unh.edu, probb@iol.unh.edu Cc: dev@dpdk.org References: <20230511091408.236638-1-juraj.linkes@pantheon.tech> <20230831100407.59865-1-juraj.linkes@pantheon.tech> <20230831100407.59865-5-juraj.linkes@pantheon.tech> Content-Language: en-US From: Yoan Picchi In-Reply-To: <20230831100407.59865-5-juraj.linkes@pantheon.tech> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 8/31/23 11:04, Juraj Linkeš wrote: > WIP: only one module is reformatted to serve as a demonstration. > > The google format is documented here [0]. > > [0]: https://google.github.io/styleguide/pyguide.html > > Signed-off-by: Juraj Linkeš > Acked-by: Jeremy Spweock > --- > dts/framework/testbed_model/node.py | 171 +++++++++++++++++++--------- > 1 file changed, 118 insertions(+), 53 deletions(-) > > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py > index 23efa79c50..619743ebe7 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -3,8 +3,13 @@ > # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. > # Copyright(c) 2022-2023 University of New Hampshire > > -""" > -A node is a generic host that DTS connects to and manages. > +"""Common functionality for node management. > + > +There's a base class, Node, that's supposed to be extended by other classes This comment and all the following ones are all something of a nitpick. This sounds too passive. Why not having something simpler like: The virtual class Node is meant to be extended by other classes with functionality specific to that node type. > +with functionality specific to that node type. > +The only part that can be used standalone is the Node.skip_setup static method, > +which is a decorator used to skip method execution if skip_setup is passed > +by the user on the cmdline or in an env variable. I'd extend env to the full word as this is meant to go in the documentation. > """ > > from abc import ABC > @@ -35,10 +40,26 @@ > > > class Node(ABC): > - """ > - Basic class for node management. This class implements methods that > - manage a node, such as information gathering (of CPU/PCI/NIC) and > - environment setup. > + """The base class for node management. > + > + It shouldn't be instantiated, but rather extended. > + It implements common methods to manage any node: > + > + * connection to the node > + * information gathering of CPU > + * hugepages setup > + > + Arguments: > + node_config: The config from the input configuration file. > + > + Attributes: > + main_session: The primary OS-agnostic remote session used > + to communicate with the node. > + config: The configuration used to create the node. > + name: The name of the node. > + lcores: The list of logical cores that DTS can use on the node. > + It's derived from logical cores present on the node and user configuration. > + ports: The ports of this node specified in user configuration. > """ > > main_session: OSSession > @@ -77,9 +98,14 @@ def _init_ports(self) -> None: > self.configure_port_state(port) > > def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > - """ > - Perform the execution setup that will be done for each execution > - this node is part of. > + """Execution setup steps. > + > + Configure hugepages and call self._set_up_execution where > + the rest of the configuration steps (if any) are implemented. > + > + Args: > + execution_config: The execution configuration according to which > + the setup steps will be taken. > """ > self._setup_hugepages() > self._set_up_execution(execution_config) > @@ -88,58 +114,78 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > self.virtual_devices.append(VirtualDevice(vdev)) > > def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional execution setup steps for derived classes. > + > + Derived classes should overwrite this > + if they want to add additional execution setup steps. I'd probably use need or require instead of want (it's the dev that wants, not the class) > """ > > def tear_down_execution(self) -> None: > - """ > - Perform the execution teardown that will be done after each execution > - this node is part of concludes. > + """Execution teardown steps. > + > + There are currently no common execution teardown steps > + common to all DTS node types. > """ > self.virtual_devices = [] > self._tear_down_execution() > > def _tear_down_execution(self) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional execution teardown steps for derived classes. > + > + Derived classes should overwrite this > + if they want to add additional execution teardown steps. > """ > > def set_up_build_target( > self, build_target_config: BuildTargetConfiguration > ) -> None: > - """ > - Perform the build target setup that will be done for each build target > - tested on this node. > + """Build target setup steps. > + > + There are currently no common build target setup steps > + common to all DTS node types. > + > + Args: > + build_target_config: The build target configuration according to which > + the setup steps will be taken. > """ > self._set_up_build_target(build_target_config) > > def _set_up_build_target( > self, build_target_config: BuildTargetConfiguration > ) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional build target setup steps for derived classes. > + > + Derived classes should optionally overwrite this Is there a reason for the "optionally" here when it's absent in all the other functions? > + if they want to add additional build target setup steps. > """ > > def tear_down_build_target(self) -> None: > - """ > - Perform the build target teardown that will be done after each build target > - tested on this node. > + """Build target teardown steps. > + > + There are currently no common build target teardown steps > + common to all DTS node types. > """ > self._tear_down_build_target() > > def _tear_down_build_target(self) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional build target teardown steps for derived classes. > + > + Derived classes should overwrite this > + if they want to add additional build target teardown steps. > """ > > def create_session(self, name: str) -> OSSession: > - """ > - Create and return a new OSSession tailored to the remote OS. > + """Create and return a new OS-agnostic remote session. > + > + The returned session won't be used by the node creating it. > + The session must be used by the caller. > + Will be cleaned up automatically. I had a double take reading this before I understood that the subject was the previously mentioned session. I'd recommend to either add "it" or "the session". Also, it will be cleaned automatically... when? When I create a new session? when the node is deleted? > + > + Args: > + name: The name of the session. > + > + Returns: > + A new OS-agnostic remote session. > """ > session_name = f"{self.name} {name}" > connection = create_session( > @@ -186,14 +232,24 @@ def filter_lcores( > filter_specifier: LogicalCoreCount | LogicalCoreList, > ascending: bool = True, > ) -> list[LogicalCore]: > - """ > - Filter the LogicalCores found on the Node according to > - a LogicalCoreCount or a LogicalCoreList. > + """Filter the node's logical cores that DTS can use. > > - If ascending is True, use cores with the lowest numerical id first > - and continue in ascending order. If False, start with the highest > - id and continue in descending order. This ordering affects which > - sockets to consider first as well. > + Logical cores that DTS can use are ones that are present on the node, > + but filtered according to user config. > + The filter_specifier will filter cores from those logical cores. > + > + Args: > + filter_specifier: Two different filters can be used, one that specifies > + the number of logical cores per core, cores per socket and > + the number of sockets, > + the other that specifies a logical core list. > + ascending: If True, use cores with the lowest numerical id first > + and continue in ascending order. If False, start with the highest > + id and continue in descending order. This ordering affects which > + sockets to consider first as well. > + > + Returns: > + A list of logical cores. > """ > self._logger.debug(f"Filtering {filter_specifier} from {self.lcores}.") > return lcore_filter( > @@ -203,17 +259,14 @@ def filter_lcores( > ).filter() > > def _get_remote_cpus(self) -> None: > - """ > - Scan CPUs in the remote OS and store a list of LogicalCores. > - """ > + """Scan CPUs in the remote OS and store a list of LogicalCores.""" > self._logger.info("Getting CPU information.") > self.lcores = self.main_session.get_remote_cpus(self.config.use_first_core) > > def _setup_hugepages(self): > - """ > - Setup hugepages on the Node. Different architectures can supply different > - amounts of memory for hugepages and numa-based hugepage allocation may need > - to be considered. > + """Setup hugepages on the Node. > + > + Configure the hugepages only if they're specified in user configuration. > """ > if self.config.hugepages: > self.main_session.setup_hugepages( > @@ -221,8 +274,11 @@ def _setup_hugepages(self): > ) > > def configure_port_state(self, port: Port, enable: bool = True) -> None: > - """ > - Enable/disable port. > + """Enable/disable port. > + > + Args: > + port: The port to enable/disable. > + enable: True to enable, false to disable. > """ > self.main_session.configure_port_state(port, enable) > > @@ -232,15 +288,19 @@ def configure_port_ip_address( > port: Port, > delete: bool = False, > ) -> None: > - """ > - Configure the IP address of a port on this node. > + """Add an IP address to a port on this node. > + > + Args: > + address: The IP address with mask in CIDR format. > + Can be either IPv4 or IPv6. > + port: The port to which to add the address. > + delete: If True, will delete the address from the port > + instead of adding it. > """ > self.main_session.configure_port_ip_address(address, port, delete) > > def close(self) -> None: > - """ > - Close all connections and free other resources. > - """ > + """Close all connections and free other resources.""" > if self.main_session: > self.main_session.close() > for session in self._other_sessions: > @@ -249,6 +309,11 @@ def close(self) -> None: > > @staticmethod > def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]: > + """A decorator that skips the decorated function. > + > + When used, the decorator executes an empty lambda function > + instead of the decorated function. > + """ > if SETTINGS.skip_setup: > return lambda *args: None > else: