From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
To: Luca Vizzarro <Luca.Vizzarro@arm.com>
Cc: Jeremy Spewock <jspewock@iol.unh.edu>,
dev@dpdk.org, Jack Bond-Preston <jack.bond-preston@arm.com>,
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Subject: Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
Date: Thu, 11 Apr 2024 12:30:58 +0200 [thread overview]
Message-ID: <CAOb5WZY5iH_9n_RTL+9AwsOj8o=yEbXLFm12NjYP-VA2BU2LtQ@mail.gmail.com> (raw)
In-Reply-To: <1db1b2b8-fac0-41ad-9ba2-911365385a9b@arm.com>
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. :-)
next prev parent reply other threads:[~2024-04-11 10:31 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 19:04 [PATCH 0/6] dts: add testpmd params and statefulness Luca Vizzarro
2024-03-26 19:04 ` [PATCH 1/6] dts: add parameters data structure Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-09 15:52 ` Luca Vizzarro
2024-04-09 12:10 ` Juraj Linkeš
2024-04-09 16:28 ` Luca Vizzarro
2024-04-10 9:15 ` Juraj Linkeš
2024-04-10 9:51 ` Luca Vizzarro
2024-04-10 10:04 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 2/6] dts: use Params for interactive shells Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-09 14:56 ` Juraj Linkeš
2024-04-10 9:34 ` Luca Vizzarro
2024-04-10 9:58 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 3/6] dts: add testpmd shell params Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-09 16:37 ` Juraj Linkeš
2024-04-10 10:49 ` Luca Vizzarro
2024-04-10 13:17 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 4/6] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-04-09 19:12 ` Juraj Linkeš
2024-04-10 10:53 ` Luca Vizzarro
2024-04-10 13:18 ` Juraj Linkeš
2024-04-26 18:06 ` Jeremy Spewock
2024-04-29 7:45 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 5/6] dts: add statefulness to InteractiveShell Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-10 6:53 ` Juraj Linkeš
2024-04-10 11:27 ` Luca Vizzarro
2024-04-10 13:35 ` Juraj Linkeš
2024-04-10 14:07 ` Luca Vizzarro
2024-04-12 12:33 ` Juraj Linkeš
2024-04-29 14:48 ` Jeremy Spewock
2024-03-26 19:04 ` [PATCH 6/6] dts: add statefulness to TestPmdShell Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-10 7:41 ` Juraj Linkeš
2024-04-10 11:35 ` Luca Vizzarro
2024-04-11 10:30 ` Juraj Linkeš [this message]
2024-04-11 11:47 ` Luca Vizzarro
2024-04-11 12:13 ` Juraj Linkeš
2024-04-11 13:59 ` Luca Vizzarro
2024-04-26 18:06 ` Jeremy Spewock
2024-04-29 12:06 ` Juraj Linkeš
2024-04-10 7:50 ` Juraj Linkeš
2024-04-10 11:37 ` Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 0/8] dts: add testpmd params Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 1/8] dts: add params manipulation module Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 3/8] dts: refactor EalParams Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 4/8] dts: remove module-wide imports Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 5/8] dts: add testpmd shell params Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 7/8] dts: rework interactive shells Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOb5WZY5iH_9n_RTL+9AwsOj8o=yEbXLFm12NjYP-VA2BU2LtQ@mail.gmail.com' \
--to=juraj.linkes@pantheon.tech \
--cc=Luca.Vizzarro@arm.com \
--cc=dev@dpdk.org \
--cc=honnappa.nagarahalli@arm.com \
--cc=jack.bond-preston@arm.com \
--cc=jspewock@iol.unh.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).