From: Luca Vizzarro <Luca.Vizzarro@arm.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
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:47:04 +0100 [thread overview]
Message-ID: <5d35012d-0ffd-4c23-ad0c-8315453b8c9e@arm.com> (raw)
In-Reply-To: <CAOb5WZY5iH_9n_RTL+9AwsOj8o=yEbXLFm12NjYP-VA2BU2LtQ@mail.gmail.com>
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.
next prev parent reply other threads:[~2024-04-11 11:47 UTC|newest]
Thread overview: 55+ 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š
2024-04-11 11:47 ` Luca Vizzarro [this message]
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
2024-05-22 15:59 ` [PATCH v2 0/8] dts: add testpmd params Nicholas Pratte
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=5d35012d-0ffd-4c23-ad0c-8315453b8c9e@arm.com \
--to=luca.vizzarro@arm.com \
--cc=dev@dpdk.org \
--cc=honnappa.nagarahalli@arm.com \
--cc=jack.bond-preston@arm.com \
--cc=jspewock@iol.unh.edu \
--cc=juraj.linkes@pantheon.tech \
/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).