Re: [PATCH v3 7/8] dts: rework interactive shells

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 17. 6. 2024 14:13, Luca Vizzarro wrote:
> On 06/06/2024 19:03, Juraj Linkeš wrote:
>>> +class DPDKShell(InteractiveShell, ABC):
>>> +    """The base class for managing DPDK-based interactive shells.
>>> +
>>> +    This class shouldn't be instantiated directly, but instead be 
>>> extended.
>>> +    It automatically injects computed EAL parameters based on the 
>>> node in the
>>> +    supplied app parameters.
>>> +    """
>>> +
>>> +    _node: SutNode
>>
>> Same here, better to be explicit with _sut_node.
> 
> This should not be changed as it's just overriding the type of the 
> parent's attribute.
> 

Ah, right, thanks for pointing this out.

>>> +    _app_params: EalParams
>>> +
>>> +    _lcore_filter_specifier: LogicalCoreCount | LogicalCoreList
>>> +    _ascending_cores: bool
>>> +    _append_prefix_timestamp: bool
>>> +
>>> +    def __init__(
>>> +        self,
>>> +        node: SutNode,
>>> +        app_params: EalParams,
>>> +        privileged: bool = True,
>>> +        timeout: float = SETTINGS.timeout,
>>> +        lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = 
>>> LogicalCoreCount(),
>>> +        ascending_cores: bool = True,
>>> +        append_prefix_timestamp: bool = True,
>>> +        start_on_init: bool = True,
>>> +    ) -> None:
>>> +        """Overrides 
>>> :meth:`~.interactive_shell.InteractiveShell.__init__`."""

I just noticed this says Overrides, but it should be extends with a 
brief note on what it's extending the constructor with.

>>> +        self._lcore_filter_specifier = lcore_filter_specifier
>>> +        self._ascending_cores = ascending_cores
>>> +        self._append_prefix_timestamp = append_prefix_timestamp
>>> +
>>> +        super().__init__(node, app_params, privileged, timeout, 
>>> start_on_init)
>>> +
>>> +    def _post_init(self):
>>> +        """Computes EAL params based on the node capabilities before 
>>> start."""
>>
>> We could just put this before calling super().__init__() in this class 
>> if we update path some other way, right? It's probably better to 
>> override the class method (_update_path()) in subclasses than having 
>> this _post_init() method.
> 
> It's more complicated than this. The ultimate parent (InteractiveShell) 
> is what sets all the common attributes. This needs to happen before 
> compute_eal_params and updating the path with the dpdk folder. This 
> wouldn't be a problem if we were to call super().__init__ at the 
> beginning. But that way we'd lose the ability to automatically start the 
> shell though.

Yes, that's why I proposed a solution that takes that into account. :-)

If we override _update_path() and change it to a regular method, we can 
just do:

         self._lcore_filter_specifier = lcore_filter_specifier
         self._ascending_cores = ascending_cores
         self._append_prefix_timestamp = append_prefix_timestamp

         # Here it's clear we don't need the parent to set the 
attributes as we have access to them.
	self._app_params = compute_eal_params(
             node,
             app_params,
             lcore_filter_specifier,
             ascending_cores,
             append_prefix_timestamp,
         )

         super().__init__(node, app_params, privileged, timeout, 
start_on_init)

     # no longer a class method, it'll create the path instance variable 
(starting with the value assigned to the path class variable) and we can 
access self._node
     def _update_path(self, path: PurePath):
         """Updates the path."""
         self.path = self._node.remote_dpdk_build_dir.joinpath(self.path))


As far as I can tell, this does the same thing without using _post_init 
and has the added benefit of overriding what we expect the subclasses to 
override (the path, as that's what should be different across shells).