[6/6] dts: add statefulness to TestPmdShell

  This commit provides a state container for TestPmdShell. It currently
only indicates whether the packet forwarding has started
or not, and the number of ports which were given to the shell.

This also fixes the behaviour of `wait_link_status_up` to use the
command timeout as inherited from InteractiveShell.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------
 1 file changed, 28 insertions(+), 13 deletions(-)
  

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit provides a state container for TestPmdShell. It currently
> only indicates whether the packet forwarding has started
> or not, and the number of ports which were given to the shell.
>
> This also fixes the behaviour of `wait_link_status_up` to use the
> command timeout as inherited from InteractiveShell.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
<snip>
> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>          if self._app_args.app_params is None:
>              self._app_args.app_params = TestPmdParameters()
>
> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
> +

This is tricky because ideally we wouldn't have the assertion here,
but I understand why it is needed because Eal parameters have app args
which can be any instance of params. I'm not sure of the best way to
solve this, because making testpmd parameters extend from eal would
break the general scheme that you have in place, and having an
extension of EalParameters that enforces this app_args is
TestPmdParameters would solve the issues, but might be a little
clunky. Is there a way we can use a generic to get python to just
understand that, in this case, this will always be TestPmdParameters?
If not I might prefer making a private class where this is
TestPmdParameters, just because there aren't really any other
assertions that we use elsewhere and an unexpected exception from this
(even though I don't think that can happen) could cause people some
issues.

It might be the case that an assertion is the easiest way to deal with
it though, what do you think?

> +        if self._app_args.app_params.auto_start:
> +            self.state.packet_forwarding_started = True
> +
> +        if self._app_args.ports is not None:
> +            self.state.number_of_ports = len(self._app_args.ports)
>
>          super()._start_application(get_privileged_command)
>
<snip>
> 2.34.1
>
  

On Thu, Mar 28, 2024 at 5:49 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
> >
> > This commit provides a state container for TestPmdShell. It currently
> > only indicates whether the packet forwarding has started
> > or not, and the number of ports which were given to the shell.
> >
> > This also fixes the behaviour of `wait_link_status_up` to use the
> > command timeout as inherited from InteractiveShell.
> >
> > Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> > Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> <snip>
> > @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> >          if self._app_args.app_params is None:
> >              self._app_args.app_params = TestPmdParameters()
> >
> > -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> > +        assert isinstance(self._app_args.app_params, TestPmdParameters)
> > +
>
> This is tricky because ideally we wouldn't have the assertion here,
> but I understand why it is needed because Eal parameters have app args
> which can be any instance of params. I'm not sure of the best way to
> solve this, because making testpmd parameters extend from eal would
> break the general scheme that you have in place, and having an
> extension of EalParameters that enforces this app_args is
> TestPmdParameters would solve the issues, but might be a little
> clunky. Is there a way we can use a generic to get python to just
> understand that, in this case, this will always be TestPmdParameters?
> If not I might prefer making a private class where this is
> TestPmdParameters, just because there aren't really any other
> assertions that we use elsewhere and an unexpected exception from this
> (even though I don't think that can happen) could cause people some
> issues.
>
> It might be the case that an assertion is the easiest way to deal with
> it though, what do you think?
>

We could change the signature (just the type of app_args) of the init
method - I think we should be able to create a type that's
EalParameters with .app_params being TestPmdParameters or None. The
init method would just call super().

Something like the above is basically necessary with inheritance where
subclasses are all extensions (not just implementations) of the
superclass (having differences in API).

> > +        if self._app_args.app_params.auto_start:
> > +            self.state.packet_forwarding_started = True
> > +
> > +        if self._app_args.ports is not None:
> > +            self.state.number_of_ports = len(self._app_args.ports)
> >
> >          super()._start_application(get_privileged_command)
> >
> <snip>
> > 2.34.1
> >
  

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit provides a state container for TestPmdShell. It currently
> only indicates whether the packet forwarding has started
> or not, and the number of ports which were given to the shell.
>

A reminder, the commit message should explain why we're doing this
change, not what the change is.

> This also fixes the behaviour of `wait_link_status_up` to use the
> command timeout as inherited from InteractiveShell.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index a823dc53be..ea1d254f86 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -678,19 +678,27 @@ def __str__(self) -> str:
>          return self.pci_address
>
>
> +@dataclass(slots=True)
> +class TestPmdState:
> +    """Session state container."""
> +
> +    #:
> +    packet_forwarding_started: bool = False

The same question as in the previous patch, do you anticipate this
being needed and should we add this only when it's actually used?

> +
> +    #: The number of ports which were allowed on the command-line when testpmd was started.
> +    number_of_ports: int = 0
> +
> +
>  class TestPmdShell(InteractiveShell):
>      """Testpmd interactive shell.
>
>      The testpmd shell users should never use
>      the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
>      call specialized methods. If there isn't one that satisfies a need, it should be added.
> -
> -    Attributes:
> -        number_of_ports: The number of ports which were allowed on the command-line when testpmd
> -            was started.
>      """
>
> -    number_of_ports: int
> +    #: Current state
> +    state: TestPmdState = TestPmdState()

Assigning a value makes this a class variable, shared across all
instances. This should be initialized in __init__().

But do we actually want to do this via composition? We'd need to
access the attributes via .state all the time and I don't really like
that. We could just put them into TestPmdShell directly, initializing
them in __init__().

>
>      #: The path to the testpmd executable.
>      path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
  

On 10/04/2024 08:41, Juraj Linkeš wrote:
>> <snip>
>>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>>>           if self._app_args.app_params is None:
>>>               self._app_args.app_params = TestPmdParameters()
>>>
>>> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
>>> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
>>> +
>>
>> This is tricky because ideally we wouldn't have the assertion here,
>> but I understand why it is needed because Eal parameters have app args
>> which can be any instance of params. I'm not sure of the best way to
>> solve this, because making testpmd parameters extend from eal would
>> break the general scheme that you have in place, and having an
>> extension of EalParameters that enforces this app_args is
>> TestPmdParameters would solve the issues, but might be a little
>> clunky. Is there a way we can use a generic to get python to just
>> understand that, in this case, this will always be TestPmdParameters?
>> If not I might prefer making a private class where this is
>> TestPmdParameters, just because there aren't really any other
>> assertions that we use elsewhere and an unexpected exception from this
>> (even though I don't think that can happen) could cause people some
>> issues.
>>
>> It might be the case that an assertion is the easiest way to deal with
>> it though, what do you think?
>>
> 
> We could change the signature (just the type of app_args) of the init
> method - I think we should be able to create a type that's
> EalParameters with .app_params being TestPmdParameters or None. The
> init method would just call super().
> 
> Something like the above is basically necessary with inheritance where
> subclasses are all extensions (not just implementations) of the
> superclass (having differences in API).
> 

I believe this is indeed a tricky one. But, unfortunately, I am not 
understanding the solution that is being proposed. To me, it just feels 
like using a generic factory like:

   self.sut_node.create_interactive_shell(..)

is one of the reasons to bring in the majority of these complexities.

What do you mean by creating this new type that combines EalParams and 
TestPmdParams?
  

On 10/04/2024 08:50, Juraj Linkeš wrote:
> On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>>
>> This commit provides a state container for TestPmdShell. It currently
>> only indicates whether the packet forwarding has started
>> or not, and the number of ports which were given to the shell.
>>
> 
> A reminder, the commit message should explain why we're doing this
> change, not what the change is.
> 
>> This also fixes the behaviour of `wait_link_status_up` to use the
>> command timeout as inherited from InteractiveShell.
>>
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> ---
>>   dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------
>>   1 file changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
>> index a823dc53be..ea1d254f86 100644
>> --- a/dts/framework/remote_session/testpmd_shell.py
>> +++ b/dts/framework/remote_session/testpmd_shell.py
>> @@ -678,19 +678,27 @@ def __str__(self) -> str:
>>           return self.pci_address
>>
>>
>> +@dataclass(slots=True)
>> +class TestPmdState:
>> +    """Session state container."""
>> +
>> +    #:
>> +    packet_forwarding_started: bool = False
> 
> The same question as in the previous patch, do you anticipate this
> being needed and should we add this only when it's actually used?
> 

As answered in the previous patch. We can always drop it and do it as 
needed of course.

>> +
>> +    #: The number of ports which were allowed on the command-line when testpmd was started.
>> +    number_of_ports: int = 0
>> +
>> +
>>   class TestPmdShell(InteractiveShell):
>>       """Testpmd interactive shell.
>>
>>       The testpmd shell users should never use
>>       the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather
>>       call specialized methods. If there isn't one that satisfies a need, it should be added.
>> -
>> -    Attributes:
>> -        number_of_ports: The number of ports which were allowed on the command-line when testpmd
>> -            was started.
>>       """
>>
>> -    number_of_ports: int
>> +    #: Current state
>> +    state: TestPmdState = TestPmdState()
> 
> Assigning a value makes this a class variable, shared across all
> instances. This should be initialized in __init__().
> 
> But do we actually want to do this via composition? We'd need to
> access the attributes via .state all the time and I don't really like
> that. We could just put them into TestPmdShell directly, initializing
> them in __init__().

No problem. I separated them in fear of bloating TestPmdShell. But I 
agree on the bother of adding .state

>>
>>       #: The path to the testpmd executable.
>>       path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
  

I overlooked this reply initially.

On Wed, Apr 10, 2024 at 1:35 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 10/04/2024 08:41, Juraj Linkeš wrote:
> >> <snip>
> >>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
> >>>           if self._app_args.app_params is None:
> >>>               self._app_args.app_params = TestPmdParameters()
> >>>
> >>> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
> >>> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
> >>> +
> >>
> >> This is tricky because ideally we wouldn't have the assertion here,
> >> but I understand why it is needed because Eal parameters have app args
> >> which can be any instance of params. I'm not sure of the best way to
> >> solve this, because making testpmd parameters extend from eal would
> >> break the general scheme that you have in place, and having an
> >> extension of EalParameters that enforces this app_args is
> >> TestPmdParameters would solve the issues, but might be a little
> >> clunky. Is there a way we can use a generic to get python to just
> >> understand that, in this case, this will always be TestPmdParameters?
> >> If not I might prefer making a private class where this is
> >> TestPmdParameters, just because there aren't really any other
> >> assertions that we use elsewhere and an unexpected exception from this
> >> (even though I don't think that can happen) could cause people some
> >> issues.
> >>
> >> It might be the case that an assertion is the easiest way to deal with
> >> it though, what do you think?
> >>
> >
> > We could change the signature (just the type of app_args) of the init
> > method - I think we should be able to create a type that's
> > EalParameters with .app_params being TestPmdParameters or None. The
> > init method would just call super().
> >
> > Something like the above is basically necessary with inheritance where
> > subclasses are all extensions (not just implementations) of the
> > superclass (having differences in API).
> >
>
> I believe this is indeed a tricky one. But, unfortunately, I am not
> understanding the solution that is being proposed. To me, it just feels
> like using a generic factory like:
>
>    self.sut_node.create_interactive_shell(..)
>
> is one of the reasons to bring in the majority of these complexities.
>

I've been thinking about these interactive shell constructors for some
time and I think the factory pattern is not well suitable for this.
Factories work well with classes with the same API (i.e.
implementations of abstract classes that don't add anything extra),
but are much less useful when dealing with classes with different
behaviors, such as the interactive shells. We see this here, different
apps are going to require different args and that alone kinda breaks
the factory pattern. I think we'll need to either ditch these
factories and instead just have methods that return the proper shell
(and the methods would only exist in classes where they belong, e.g.
testpmd only makes sense on an SUT). Or we could overload each factory
(the support has only been added in 3.11 with @typing.overload, but is
also available in typing_extensions, so we would be able to use it
with the extra dependency) where different signatures would return
different objects. In both cases the caller won't have to import the
class and the method signature is going to be clearer.

We have this pattern with sut/tg nodes. I decided to move away from
the node factory because it didn't add much and in fact the code was
only clunkier. The interactive shell is not quite the same, as the
shells are not standalone in the same way the nodes are (the shells
are tied to nodes). Let me know what you think about all this - both
Luca and Jeremy.

> What do you mean by creating this new type that combines EalParams and
> TestPmdParams?

Let me illustrate this on the TestPmdShell __init__() method I had in mind:

def __init__(self, interactive_session: SSHClient,
        logger: DTSLogger,
        get_privileged_command: Callable[[str], str] | None,
        app_args: EalTestPmdParams | None = None,
        timeout: float = SETTINGS.timeout,
    ) -> None:
    super().__init__(interactive_session, logger, get_privileged_command)
    self.state = TestPmdState()

Where EalTestPmdParams would be something that enforces that
app_args.app_params is of the TestPmdParameters type.

But thinking more about this, we're probably better off switching the
params composition. Instead of TestPmdParameters being part of
EalParameters, we do it the other way around. This way the type of
app_args could just be TestPmdParameters and the types should work.
Or we pass the args separately, but that would likely require ditching
the factories and replacing them with methods (or overloading them).

And hopefully the imports won't be impossible to solve. :-)
  

On 11/04/2024 11:30, Juraj Linkeš wrote:
> I've been thinking about these interactive shell constructors for some
> time and I think the factory pattern is not well suitable for this.
> Factories work well with classes with the same API (i.e.
> implementations of abstract classes that don't add anything extra),
> but are much less useful when dealing with classes with different
> behaviors, such as the interactive shells. We see this here, different
> apps are going to require different args and that alone kinda breaks
> the factory pattern. I think we'll need to either ditch these
> factories and instead just have methods that return the proper shell
> (and the methods would only exist in classes where they belong, e.g.
> testpmd only makes sense on an SUT). Or we could overload each factory
> (the support has only been added in 3.11 with @typing.overload, but is
> also available in typing_extensions, so we would be able to use it
> with the extra dependency) where different signatures would return
> different objects. In both cases the caller won't have to import the
> class and the method signature is going to be clearer.
> 
> We have this pattern with sut/tg nodes. I decided to move away from
> the node factory because it didn't add much and in fact the code was
> only clunkier. The interactive shell is not quite the same, as the
> shells are not standalone in the same way the nodes are (the shells
> are tied to nodes). Let me know what you think about all this - both
> Luca and Jeremy.

When writing this series, I went down the path of creating a 
`create_testpmd_shell` method at some point as a solution to these 
problems. Realising after that it may be too big of a change, and 
possibly best left to a discussion exactly like this one.

Generics used at this level may be a bit too much, especially for 
Python, as support is not *that* great. I am of the opinion that having 
a dedicated wrapper is easier for the developer and the user. Generics 
are not needed to this level anyways, as we have a limited selection of 
shells that are actually going to be used.

We can also swap the wrapping process to simplify things, instead of:

   shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)

do:

   shell = TestPmdShell(self.sut_node, ..)

Let the Shell class ingest the node, and not the other way round.

The current approach appears to me to be top-down instead of bottom-up. 
We take the most abstracted part and we work our way down. But all we 
want is concreteness to the end user (developer).

> Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> 
> def __init__(self, interactive_session: SSHClient,
>          logger: DTSLogger,
>          get_privileged_command: Callable[[str], str] | None,
>          app_args: EalTestPmdParams | None = None,
>          timeout: float = SETTINGS.timeout,
>      ) -> None:
>      super().__init__(interactive_session, logger, get_privileged_command)
>      self.state = TestPmdState()
> 
> Where EalTestPmdParams would be something that enforces that
> app_args.app_params is of the TestPmdParameters type.
> 
> But thinking more about this, we're probably better off switching the
> params composition. Instead of TestPmdParameters being part of
> EalParameters, we do it the other way around. This way the type of
> app_args could just be TestPmdParameters and the types should work.
> Or we pass the args separately, but that would likely require ditching
> the factories and replacing them with methods (or overloading them).
> 
> And hopefully the imports won't be impossible to solve. :-)

It is what I feared, and I think it may become even more convoluted. As 
you said, ditching the factories will simplify things and make it more 
straightforward. So, we wouldn't find ourselves in problems like these.

I don't have a strong preference in approach between:
* overloading node methods
* dedicated node methods
* let the shells ingest nodes instead

But if I were to give priority, I'd take it from last to first. Letting 
shells ingest nodes will decouple the situation adding an extra step of 
simplification. I may not see the full picture though. The two are 
reasonable but, having a dedicated node method will stop the requirement 
to import the shell we need, and it's pretty much equivalent... but 
overloading also is very new to Python, so I may prefer to stick to more 
established.

Letting TestPmdParams take EalParams, instead of the other way around, 
would naturally follow the bottom-up approach too. Allowing Params to 
arbitrarily append string arguments – as proposed, would also allow 
users to use a plain (EalParams + string). So sounds like a good 
approach overall.
  

On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 11/04/2024 11:30, Juraj Linkeš wrote:
> > I've been thinking about these interactive shell constructors for some
> > time and I think the factory pattern is not well suitable for this.
> > Factories work well with classes with the same API (i.e.
> > implementations of abstract classes that don't add anything extra),
> > but are much less useful when dealing with classes with different
> > behaviors, such as the interactive shells. We see this here, different
> > apps are going to require different args and that alone kinda breaks
> > the factory pattern. I think we'll need to either ditch these
> > factories and instead just have methods that return the proper shell
> > (and the methods would only exist in classes where they belong, e.g.
> > testpmd only makes sense on an SUT). Or we could overload each factory
> > (the support has only been added in 3.11 with @typing.overload, but is
> > also available in typing_extensions, so we would be able to use it
> > with the extra dependency) where different signatures would return
> > different objects. In both cases the caller won't have to import the
> > class and the method signature is going to be clearer.
> >
> > We have this pattern with sut/tg nodes. I decided to move away from
> > the node factory because it didn't add much and in fact the code was
> > only clunkier. The interactive shell is not quite the same, as the
> > shells are not standalone in the same way the nodes are (the shells
> > are tied to nodes). Let me know what you think about all this - both
> > Luca and Jeremy.
>
> When writing this series, I went down the path of creating a
> `create_testpmd_shell` method at some point as a solution to these
> problems. Realising after that it may be too big of a change, and
> possibly best left to a discussion exactly like this one.
>

The changes we discuss below don't seem that big. What do you think,
do we just add another patch to the series?

> Generics used at this level may be a bit too much, especially for
> Python, as support is not *that* great. I am of the opinion that having
> a dedicated wrapper is easier for the developer and the user. Generics
> are not needed to this level anyways, as we have a limited selection of
> shells that are actually going to be used.
>
> We can also swap the wrapping process to simplify things, instead of:
>
>    shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)
>
> do:
>
>    shell = TestPmdShell(self.sut_node, ..)
>
> Let the Shell class ingest the node, and not the other way round.
>

I thought about this a bit as well, it's a good approach. The current
design is top-down, as you say, in that "I have a node and I do things
with the node, including starting testpmd on the node". But it could
also be "I have a node, but I also have other non-node resources at my
disposal and it's up to me how I utilize those". If we can make the
imports work then this is likely the best option.

> The current approach appears to me to be top-down instead of bottom-up.
> We take the most abstracted part and we work our way down. But all we
> want is concreteness to the end user (developer).
>
> > Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> >
> > def __init__(self, interactive_session: SSHClient,
> >          logger: DTSLogger,
> >          get_privileged_command: Callable[[str], str] | None,
> >          app_args: EalTestPmdParams | None = None,
> >          timeout: float = SETTINGS.timeout,
> >      ) -> None:
> >      super().__init__(interactive_session, logger, get_privileged_command)
> >      self.state = TestPmdState()
> >
> > Where EalTestPmdParams would be something that enforces that
> > app_args.app_params is of the TestPmdParameters type.
> >
> > But thinking more about this, we're probably better off switching the
> > params composition. Instead of TestPmdParameters being part of
> > EalParameters, we do it the other way around. This way the type of
> > app_args could just be TestPmdParameters and the types should work.
> > Or we pass the args separately, but that would likely require ditching
> > the factories and replacing them with methods (or overloading them).
> >
> > And hopefully the imports won't be impossible to solve. :-)
>
> It is what I feared, and I think it may become even more convoluted. As
> you said, ditching the factories will simplify things and make it more
> straightforward. So, we wouldn't find ourselves in problems like these.
>
> I don't have a strong preference in approach between:
> * overloading node methods
> * dedicated node methods
> * let the shells ingest nodes instead
>
> But if I were to give priority, I'd take it from last to first. Letting
> shells ingest nodes will decouple the situation adding an extra step of
> simplification.

+1 for simplification.

> I may not see the full picture though. The two are
> reasonable but, having a dedicated node method will stop the requirement
> to import the shell we need, and it's pretty much equivalent... but
> overloading also is very new to Python, so I may prefer to stick to more
> established.
>

Let's try shells ingesting nodes if the imports work out then. If not,
we can fall back to dedicated node methods.

> Letting TestPmdParams take EalParams, instead of the other way around,
> would naturally follow the bottom-up approach too. Allowing Params to
> arbitrarily append string arguments – as proposed, would also allow
> users to use a plain (EalParams + string). So sounds like a good
> approach overall.
  

On 11/04/2024 13:13, Juraj Linkeš wrote:
> The changes we discuss below don't seem that big. What do you think,
> do we just add another patch to the series?

Sure thing, I can take this and add it to v2.

> I thought about this a bit as well, it's a good approach. The current
> design is top-down, as you say, in that "I have a node and I do things
> with the node, including starting testpmd on the node". But it could
> also be "I have a node, but I also have other non-node resources at my
> disposal and it's up to me how I utilize those". If we can make the
> imports work then this is likely the best option.
> 
> <snip>
> 
> +1 for simplification.
>
> <snip> >
> Let's try shells ingesting nodes if the imports work out then. If not,
> we can fall back to dedicated node methods.

Sounds good!
  

Apologies for being so late on the discussion, but just a few of my
thoughts:

* I think using something like overloading even though it is new to
python is completely fine because this new python version is a
dependency of  the DTS runner. The DTS runner can have bleeding-edge
requirements because we manage that through a container to make things
easier.

On Thu, Apr 11, 2024 at 8:13 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> >
> > On 11/04/2024 11:30, Juraj Linkeš wrote:
> > > I've been thinking about these interactive shell constructors for some
> > > time and I think the factory pattern is not well suitable for this.
> > > Factories work well with classes with the same API (i.e.
> > > implementations of abstract classes that don't add anything extra),
> > > but are much less useful when dealing with classes with different
> > > behaviors, such as the interactive shells. We see this here, different
> > > apps are going to require different args and that alone kinda breaks
> > > the factory pattern. I think we'll need to either ditch these
> > > factories and instead just have methods that return the proper shell
> > > (and the methods would only exist in classes where they belong, e.g.
> > > testpmd only makes sense on an SUT). Or we could overload each factory
> > > (the support has only been added in 3.11 with @typing.overload, but is
> > > also available in typing_extensions, so we would be able to use it
> > > with the extra dependency) where different signatures would return
> > > different objects. In both cases the caller won't have to import the
> > > class and the method signature is going to be clearer.
> > >
> > > We have this pattern with sut/tg nodes. I decided to move away from
> > > the node factory because it didn't add much and in fact the code was
> > > only clunkier. The interactive shell is not quite the same, as the
> > > shells are not standalone in the same way the nodes are (the shells
> > > are tied to nodes). Let me know what you think about all this - both
> > > Luca and Jeremy.
> >
> > When writing this series, I went down the path of creating a
> > `create_testpmd_shell` method at some point as a solution to these
> > problems. Realising after that it may be too big of a change, and
> > possibly best left to a discussion exactly like this one.
> >
>
> The changes we discuss below don't seem that big. What do you think,
> do we just add another patch to the series?
>
> > Generics used at this level may be a bit too much, especially for
> > Python, as support is not *that* great. I am of the opinion that having
> > a dedicated wrapper is easier for the developer and the user. Generics
> > are not needed to this level anyways, as we have a limited selection of
> > shells that are actually going to be used.
> >
> > We can also swap the wrapping process to simplify things, instead of:
> >
> >    shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)
> >
> > do:
> >
> >    shell = TestPmdShell(self.sut_node, ..)
> >
> > Let the Shell class ingest the node, and not the other way round.
> >
>
> I thought about this a bit as well, it's a good approach. The current
> design is top-down, as you say, in that "I have a node and I do things
> with the node, including starting testpmd on the node". But it could
> also be "I have a node, but I also have other non-node resources at my
> disposal and it's up to me how I utilize those". If we can make the
> imports work then this is likely the best option.

It might be me slightly stuck in the old ways of doing things, but I
might slightly favor the overloading methods approach. This is really
because, at least in my mind, the SUT node is somewhat of a central
API for the developer to use during testing, so having a method on
that API for creating a shell for you to use on the node makes sense
to me. It creates more of a "one stop shop" kind of idea where
developers have to do less reading about how to do things and can just
look at the methods of the SUT node to get what they would need.

That being said, I think in any other framework the passing of the
node into the shell would easily make more sense and I'm not opposed
to going that route either. In general, I agree that not using a
factory with a generic will make things much easier in the future.

>
> > The current approach appears to me to be top-down instead of bottom-up.
> > We take the most abstracted part and we work our way down. But all we
> > want is concreteness to the end user (developer).
> >
> > > Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> > >
> > > def __init__(self, interactive_session: SSHClient,
> > >          logger: DTSLogger,
> > >          get_privileged_command: Callable[[str], str] | None,
> > >          app_args: EalTestPmdParams | None = None,
> > >          timeout: float = SETTINGS.timeout,
> > >      ) -> None:
> > >      super().__init__(interactive_session, logger, get_privileged_command)
> > >      self.state = TestPmdState()
> > >
> > > Where EalTestPmdParams would be something that enforces that
> > > app_args.app_params is of the TestPmdParameters type.
> > >
> > > But thinking more about this, we're probably better off switching the
> > > params composition. Instead of TestPmdParameters being part of
> > > EalParameters, we do it the other way around. This way the type of
> > > app_args could just be TestPmdParameters and the types should work.
> > > Or we pass the args separately, but that would likely require ditching
> > > the factories and replacing them with methods (or overloading them).
> > >
> > > And hopefully the imports won't be impossible to solve. :-)
> >
> > It is what I feared, and I think it may become even more convoluted. As
> > you said, ditching the factories will simplify things and make it more
> > straightforward. So, we wouldn't find ourselves in problems like these.
> >
> > I don't have a strong preference in approach between:
> > * overloading node methods
> > * dedicated node methods
> > * let the shells ingest nodes instead
> >
> > But if I were to give priority, I'd take it from last to first. Letting
> > shells ingest nodes will decouple the situation adding an extra step of
> > simplification.
>
> +1 for simplification.
>
> > I may not see the full picture though. The two are
> > reasonable but, having a dedicated node method will stop the requirement
> > to import the shell we need, and it's pretty much equivalent... but
> > overloading also is very new to Python, so I may prefer to stick to more
> > established.
> >
>
> Let's try shells ingesting nodes if the imports work out then. If not,
> we can fall back to dedicated node methods.
>
> > Letting TestPmdParams take EalParams, instead of the other way around,
> > would naturally follow the bottom-up approach too. Allowing Params to
> > arbitrarily append string arguments – as proposed, would also allow
> > users to use a plain (EalParams + string). So sounds like a good
> > approach overall.

This I like a lot. We don't want to force EalParams to have
TestpmdParams nested inside of them because other DPDK apps might need
them too and this fixes the issue of always having to assert what type
of inner params you have.