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).
