Composition over inheritance in probes #454

kmazurek · 2021-03-18T17:17:05Z

Resolves: #433

Overview of changes!

Probe components

This PR introduces the concept of probe components (ProbeComponent), replacing certain functionalities previously realised through mixins (composition over inheritance). With these changes the Probe class now includes two types of components:

agents, which are subclasses of AgentComponent. These replace AgentMixin and allow for having multiple agents running as part of a single probe. In the current implementation ProviderProbe uses this logic for its ProviderAgentComponent.
api, which is of type RestApiComponent. Since this is now part of the base Probe class, both provider and requestor probes can now use the yagna REST APIs.

Flattening probe hierarchy

With these changes there are now only two concrete probe classes: ProviderProbe and RequestorProbe (so no more RequestorProbeWithApiSteps and ProviderProbeWithLogSteps). All high-level test step functions are now placed in domain-specific mixins, some of which are shared between the probe classes (see goth/runner/probe/mixin.py for more details).

goth/runner/__init__.py

Co-authored-by: azawlocki <artur.zawlocki@golem.network>

azawlocki

Good job, our class diagram looks much nicer with these changes!

I've left some remarks and questions, I think those in goth/runner/probe/agent.py need to be addressed before merging.

azawlocki · 2021-03-30T08:40:48Z

goth/runner/probe/__init__.py

+    agents: Set[AgentComponent]
+    """Set of agent components that will be started as part of this probe."""


It it were a list, not a set, we could guarantee that the agents are started in the order they appear on the list. Maybe this would be a useful feature? Just thinking aloud.

I agree, this could be useful. Please see my comment on AgentComponent.__hash__. We could replace this with an OrderedSet (though that would require pulling in an additional dependency, i.e. https://pypi.org/project/ordered-set/) or an OrderedDict.

After your suggestion on AgentComponent.__hash__ I'm going to change this to a list.

azawlocki · 2021-03-30T08:43:35Z

goth/runner/__init__.py

-        for probe in self.get_probes(probe_type=AgentMixin):
-            awaitables.append(probe.start_agent())
+        for probe in self.probes:
+            awaitables.extend([a.start() for a in probe.agents])


Maybe it would be cleaner to have a start_agents() method in Probe:

awaitables = [probe.start_agents() for probe in self.probes]

Added start_agents method in 5a4a7b8

azawlocki · 2021-03-30T09:24:54Z

goth/runner/probe/agent.py

+            log_config.base_dir = probe.container.log_config.base_dir
+
+        self.log_monitor = LogEventMonitor(self.name, log_config)
+        self._last_checked_line = -1


We don't need this line anymore.

Removed in b365b29

azawlocki · 2021-03-30T09:27:12Z

goth/runner/probe/agent.py

+        self.log_monitor = LogEventMonitor(self.name, log_config)
+        self._last_checked_line = -1
+
+    async def wait_for_log(self, pattern: str, timeout: float = 1000) -> LogEvent:


Use None as default value.

Updated in b365b29

azawlocki · 2021-03-30T09:33:51Z

goth/runner/probe/agent.py

+        return entry
+
+    def __hash__(self):
+        return hash(self.name)


This seems dangerous, why not use default __hash__() here which is based on object identity for custom classes
(https://docs.python.org/3/glossary.html#term-hashable)?

This, together with changing Probe.agents from a list to a set, was done to prevent adding the same agent type (not necessarily the same class instance) twice to a single Probe object (I assumed the same agent type would yield the same name).

We could replace this set + custom hash approach with a dict (that is: make Probe.agents a dict in which the keys are names assigned to agents). My problem with this approach is that it makes iterating over agents unnecessarily complex. Also, I think it still makes sense to keep the name property on agents, since it's used in AgentComponent itself for creating the agent's log file.

Why forbid two agent components with the same type in a probe? I can imagine a test scenario with two requestor agents running in one node. Sure, they would have different names but the same type.

Moreover, to guarantee no two agent components have the same type I think it would be more natural to define __hash__() based on the type, not only the name.

Finally, nothing prevents one from adding two agent components with the same type/name, one of them will be simply silently ignored (probably the second one, but I'm not sure). Maybe instead of overriding __hash__() for AgentComponent, Probe should have a method like this:

def add_agent(agent: AgentComponent): if any(a.name == agent.name for a in self.agents): raise SomeError(f"The probe already has an agent component named `{agent.name}`") ....

Thanks for the suggestion, I like this idea. I'll add an add_agent method to Probe.

azawlocki · 2021-03-30T09:43:15Z

test/goth/runner/probe/test_agent.py

+
+
+def test_agent_duplicate_names():
+    """Test if two agent objects with the same name will be treated as equal."""


Do we need this property?

Please see my comment on AgentComponent.__hash__.

azawlocki

LGTM!

Move REST API functionality to base probe class

c1e5340

kmazurek self-assigned this Mar 18, 2021

kmazurek added 3 commits March 24, 2021 13:59

Introduce agent components to Probe

8156353

Move daemon address setup to RestApiComponent

1b50616

Use a set for storing probe agents

4a4c94b

kmazurek force-pushed the km/probe-modules branch from c396496 to 4a4c94b Compare March 24, 2021 17:09

Reduce probe class hierarchy

fe4f9d1

kmazurek changed the title ~~Move REST API functionality to base probe class~~ Composition over inheritance in probes Mar 24, 2021

Merge branch 'master' into km/probe-modules

fa77302

kmazurek marked this pull request as ready for review March 25, 2021 13:01

kmazurek requested a review from azawlocki March 25, 2021 13:02

azawlocki reviewed Mar 26, 2021

View reviewed changes

goth/runner/__init__.py Outdated Show resolved Hide resolved

kmazurek and others added 5 commits March 26, 2021 15:33

Add more logging to runner exit stack calls

f0858a4

Black formatting

55c178a

Initalise log monitor in AgentComponent __init__

44cd5fa

Update agent unit tests

3af6bb3

Fix getting probe agents in Runner

e450ea9

Co-authored-by: azawlocki <artur.zawlocki@golem.network>

kmazurek mentioned this pull request Mar 26, 2021

Add integration test that starts blender example in goth golemfactory/yapapi#281

Merged

azawlocki suggested changes Mar 30, 2021

View reviewed changes

kmazurek added 3 commits March 30, 2021 14:53

Add start_agents method to Probe

5a4a7b8

Review comments in agent.py

b365b29

Merge branch 'master' into km/probe-modules

c28cbda

azawlocki approved these changes Mar 31, 2021

View reviewed changes

Create add_agent method, use OrderedDict for storing agents

a9d0d89

kmazurek force-pushed the km/probe-modules branch from cc98cc3 to a9d0d89 Compare March 31, 2021 08:27

kmazurek merged commit 2edc941 into master Mar 31, 2021

kmazurek deleted the km/probe-modules branch March 31, 2021 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composition over inheritance in probes #454

Composition over inheritance in probes #454

kmazurek commented Mar 18, 2021 •

edited

Loading

azawlocki left a comment

azawlocki Mar 30, 2021

kmazurek Mar 30, 2021

kmazurek Mar 30, 2021 •

edited

Loading

azawlocki Mar 30, 2021

kmazurek Mar 30, 2021

azawlocki Mar 30, 2021

kmazurek Mar 30, 2021

azawlocki Mar 30, 2021

kmazurek Mar 30, 2021

azawlocki Mar 30, 2021

kmazurek Mar 30, 2021

azawlocki Mar 30, 2021 •

edited

Loading

kmazurek Mar 30, 2021

azawlocki Mar 30, 2021

kmazurek Mar 30, 2021

azawlocki left a comment

		agents: Set[AgentComponent]
		"""Set of agent components that will be started as part of this probe."""



		def test_agent_duplicate_names():
		"""Test if two agent objects with the same name will be treated as equal."""

Composition over inheritance in probes #454

Composition over inheritance in probes #454

Conversation

kmazurek commented Mar 18, 2021 • edited Loading

Probe components

Flattening probe hierarchy

azawlocki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azawlocki Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azawlocki left a comment

Choose a reason for hiding this comment

kmazurek commented Mar 18, 2021 •

edited

Loading

kmazurek Mar 30, 2021 •

edited

Loading

azawlocki Mar 30, 2021 •

edited

Loading