Rework EAS acceptance tests to inherit from LisaTest #197

bjackman · 2016-09-27T18:18:54Z

Refactor/enhance the Executor class; most importantly enable the "experiments" to be saved so that test objects can refer back to them in test_ methods
Add an EasTest class abstracting the common operations for all EAS acceptance tests. It's likely that some of this functionality (+ documentation which is not written) could go directly into LisaTest, but I haven't thought about that properly.
Rework each of the acceptance test classes to inherit from EasTest

Also some miscellaneous cleanups that seem to me to be related to this.

This commit gives Executors a list of structs containing metadata about the experiments it runs. The primary motivation for this is so that tests subclassing LisaTest can use this information in their test_* methods. For example, tests can use the 'out_dir' member to construct Trappy Ftrace objects.

…s" param On "periodic" rt-app workloads, the "tasks" param specifies how many instances of the given task configuration should be instantiated. For example, if the tests_conf contains: "wloads" : { "my_periodic" : { "type" : "rt-app", "conf" : { "class" : "periodic", "params" : { "duty_cycle_pct": 100, "duration_s" : 5, "period_ms": 10 }, "tasks" : 10 // <-- Create 10 tasks }, } then 10 tasks with the given "params" will be created. This commit adds the same functionality for tasks within a "type" of "profile. Example: "fmig" : { "type" : "rt-app", "conf" : { "class" : "profile", "params" : { "small" : { "kind" : "Periodic", "params" : { "duty_cycle_pct": 10, "duration_s": 5, "period_ms": 10, }, "tasks" : 2 }, "big" : { "kind" : "Periodic", "params" : { "duty_cycle_pct": 100, "duration_s" : 5, "period_ms": 10 }, "tasks" : 4 }, }, }, }, The above will create a workload that runs 4 100% tasks and 2 10% periodic tasks in parallel.

Re-order imports according to PEP8 [1]. Also remove duplicate `import json`. [1] https://www.python.org/dev/peps/pep-0008/#imports

This adds a class EasTest that inherits from LisaTest to provide a common set of functions for EAS acceptance tests.

Now that all the EAS acceptance tests have been ported to use the EasTest class this code is no longer needed

derkling

I love this refactoring, thanks @bjackman for the effort!
Perhaps we can improve something before merging but I think we are already in a quite good status.
Since we are changing so much, I would really like a slightly better sequencing for some patches.

derkling · 2016-09-27T22:14:56Z

libs/utils/executor.py

-                    # WORKLOAD: execution
-                    self._wload_run(exp_idx, tc, wl_idx, wload, itr_idx)
-                    exp_idx += 1
+                    self.experiments.append({


I like this idea to initialize all the confs and do the actual execution only after.
I'm wondering if it could make more sense here to use a namedtuple to represent one experiment, e.g.

from collections import namedtuple Experiment = namedtuple('Experiment', ['wload_name', 'wload', 'conf', 'iteration' ,'out_dir']) self.experiments.append(Experiment(wl_idx, wload, tc, itr_idx, os.path.join(test_dir, str(itr_idx)))

This should perhaps simplify the following code cause you can:

loop on Experiment tuples, which are passed directly to the _wload_run (with ** expansion if required)

remove some dictionary access lines by replacing them with simpler calls, e.g. exp.wload_name instead of exp['wload_name']

Nice, will give that a go.

derkling · 2016-09-27T22:16:22Z

libs/utils/executor.py

@@ -464,38 +473,38 @@ def _wload_init(self, tc, wl_idx):
        wload = self._wload_conf(wl_idx, wlspec)

        # Keep track of platform configuration
-        self.te.test_dir = '{}/{}:{}:{}'\


That's just a cosmetics chunk... can you keep them on a separate patch (maybe before)?

derkling · 2016-09-27T22:17:45Z

libs/utils/executor.py


        # Keep track of kernel configuration and version
        config = self.target.config
-        with gzip.open(os.path.join(self.te.test_dir, 'kernel.config'), 'wb') as fh:


Again, these two chunks are cosmetics... keep in the same separate patch with the previous one.

Agreed on the change to the format call, but not on the self.te.test_dir -> test_dir:

Before this change, self.te.test_dir is global state that is picked up later in _wload_run_init. I could put this change in a previous patch which also modifies _wload_run and _wload_init to take a test_dir argument, but it seems unnecessary given that it will immediately be undone...

derkling · 2016-09-27T22:19:19Z

libs/utils/executor.py

-        logging.debug(r'%14s - out_dir [%s]', 'Executor', self.te.out_dir)
-        os.system('mkdir -p ' + self.te.out_dir)
+    def _wload_run_init(self, experiment):
+        experiment['out_dir'] = _get_experiment_out_dir(experiment)


isn't the out_dir already defined for each experiment in line 149?

Yep, looks like this is leakage from another version of the commit.

derkling · 2016-09-27T22:21:37Z

libs/utils/executor.py

-                .format(exp_idx, self._exp_count,
-                        tc_idx, wl_idx,
-                        run_idx, self._iterations))
+                .format(exp_idx, self._exp_count, # todo len(self.experiments?)


Here is where we can simplify a bit using the namedtuple, maybe by also s/experiment/exp/

And the #todo, either do it or remove it from the patch ;-)

Removed the todo; it would be nice to get rid of the calculation of self._exp_count in __init__ but for now I think it's more trouble than it's worth.

Re: s/experiment/exp/: why? We aren't short on horizontal space (the format call has to be broken up, but it would be anyway), so why not be totally clear on what the variable refers to?

I agree on using clear names... just wondering is exp can be a shorter still clear name.
No main concerns if you prefer to keep experiment ;-)

derkling · 2016-09-27T22:53:48Z

tests/eas/offload_idle_pull.config

+{
+    "modules"  : [ "bl" ],
+    "tools"    : [ "rt-app" ],
+    "ftrace" : { // TODO make these inherited


How do you want to inherit these?
In any case, if we do not do it now let's remove the comment as well from the committed code.

Sorry, commit leakage again here. I was thinking about the fact that we want to specify the required events in order to avoid enormous traces, but they are the same for all the EAS tests. This is something to improve later, I think.

derkling · 2016-09-27T22:54:24Z

tests/eas/acceptance.py

-        cls.env.ftrace.stop()
-        trace = cls.env.ftrace.get_trace(cls.trace_file)
+    conf_basename = "wake_migration.config"
+    # TODO...


Oops, more commit leakage.

derkling · 2016-09-27T22:55:37Z

tests/eas/acceptance.py

+    # TODO...
+    phase_duration = WORKLOAD_DURATION_S
+
+    def _assert_switch(self, experiment, expected_switch_to, phases):


Again, this seems more an update to the test than a refactoring, isn't it?
Maybe we can still use one additional patch with these changes?

Agreed, I'll split this up.

derkling · 2016-09-27T22:57:01Z

tests/eas/acceptance.py

+    phase_duration = WORKLOAD_DURATION_S
+
+    def _assert_switch(self, experiment, expected_switch_to, phases):
+        if expected_switch_to == "big":


Cannot we really avoid this big.LITTLE-ism? Perhaps not right now... :-/

Yeah, I think this is something to remove later.

The EAS acceptance tests are all about big.LITTLE I'm afraid. They use the "bl" module of target!

derkling · 2016-09-27T22:58:25Z

tests/eas/acceptance.py

        """Wake Migration: BIG -> LITLLE: 2"""

        # little - big - little - big
        #              ^
        # We want to test that this big to little migration happens.  So we skip
        # the first two phases.
-        expected_time = self.offset + 2 * self.phase_duration


That patch is changing so much... we should really try to split it in two, or three if required, to make more easy to understand what it is doing...

Agreed, will split this patch into one that ports to EasTest and one that factors out the commonality between the switch tests.

JaviMerino · 2016-09-28T09:40:35Z

tests/eas/acceptance.py

+    Base class for EAS tests
+    """
+
+    set_is_big_little = True


This is not used anywhere in the commit. Should it be moved to the commit where it is actually used?

Ah, this is a missing feature in the EasTest class - we need to set the "is_big_little" flag. Will add the missing code, I'll put it in this commit.

bjackman

Thanks a lot for the quick review! I have to be honest, I was expecting to have a chance to do my own self-review before anyone else got looked at it (hence the "TODO" leaks), probably I shouldn't have clicked "submit"!

bjackman · 2016-09-28T09:19:04Z

libs/utils/executor.py

-                    # WORKLOAD: execution
-                    self._wload_run(exp_idx, tc, wl_idx, wload, itr_idx)
-                    exp_idx += 1
+                    self.experiments.append({


Nice, will give that a go.

bjackman · 2016-09-28T09:19:47Z

libs/utils/executor.py

@@ -464,38 +473,38 @@ def _wload_init(self, tc, wl_idx):
        wload = self._wload_conf(wl_idx, wlspec)

        # Keep track of platform configuration
-        self.te.test_dir = '{}/{}:{}:{}'\


bjackman · 2016-09-28T09:21:15Z

libs/utils/executor.py


        # Keep track of kernel configuration and version
        config = self.target.config
-        with gzip.open(os.path.join(self.te.test_dir, 'kernel.config'), 'wb') as fh:


Agreed on the change to the format call, but not on the self.te.test_dir -> test_dir:

Before this change, self.te.test_dir is global state that is picked up later in _wload_run_init. I could put this change in a previous patch which also modifies _wload_run and _wload_init to take a test_dir argument, but it seems unnecessary given that it will immediately be undone...

bjackman · 2016-09-28T09:22:16Z

libs/utils/executor.py

-        logging.debug(r'%14s - out_dir [%s]', 'Executor', self.te.out_dir)
-        os.system('mkdir -p ' + self.te.out_dir)
+    def _wload_run_init(self, experiment):
+        experiment['out_dir'] = _get_experiment_out_dir(experiment)


Yep, looks like this is leakage from another version of the commit.

bjackman · 2016-09-28T09:24:52Z

libs/utils/executor.py

@@ -376,9 +376,7 @@ def _wload_rtapp(self, wl_idx, wlspec, cpus):
        if conf['class'] == 'profile':
            params = {}
            # Load each task specification
-            for task_name in conf['params']:
-                task = conf['params'][task_name]
-                task_name = conf['prefix'] + task_name


That's a mistake. I think I'll drop this commit.

bjackman · 2016-09-28T09:43:00Z

tests/eas/offload_idle_pull.config

+{
+    "modules"  : [ "bl" ],
+    "tools"    : [ "rt-app" ],
+    "ftrace" : { // TODO make these inherited


Sorry, commit leakage again here. I was thinking about the fact that we want to specify the required events in order to avoid enormous traces, but they are the same for all the EAS tests. This is something to improve later, I think.

bjackman · 2016-09-28T09:44:05Z

tests/eas/acceptance.py

-        cls.env.ftrace.stop()
-        trace = cls.env.ftrace.get_trace(cls.trace_file)
+    conf_basename = "wake_migration.config"
+    # TODO...


Oops, more commit leakage.

bjackman · 2016-09-28T09:44:21Z

tests/eas/acceptance.py

+    # TODO...
+    phase_duration = WORKLOAD_DURATION_S
+
+    def _assert_switch(self, experiment, expected_switch_to, phases):


Agreed, I'll split this up.

bjackman · 2016-09-28T09:44:43Z

tests/eas/acceptance.py

+    phase_duration = WORKLOAD_DURATION_S
+
+    def _assert_switch(self, experiment, expected_switch_to, phases):
+        if expected_switch_to == "big":


Yeah, I think this is something to remove later.

bjackman · 2016-09-28T09:45:18Z

tests/eas/acceptance.py

        """Wake Migration: BIG -> LITLLE: 2"""

        # little - big - little - big
        #              ^
        # We want to test that this big to little migration happens.  So we skip
        # the first two phases.
-        expected_time = self.offset + 2 * self.phase_duration


Agreed, will split this patch into one that ports to EasTest and one that factors out the commonality between the switch tests.

JaviMerino · 2016-09-28T09:51:23Z

tests/eas/acceptance.py

            msg="Not all tasks are running on LITTLE cores for at least {}% of their execution time"\
-                    .format(EXPECTED_RESIDENCY_PCT))
-
+            .format(EXPECTED_RESIDENCY_PCT))


Why remove the indentation? It's a continuation line and pep8 says "Make sure to indent the continued line appropriately".

Oops, this is editor noise, will undo.

bjackman · 2016-09-28T09:52:51Z

Since this PR is enormous and I'm rapidly discovering that the GitHub interface is fair from ideal I will rework this and submit a separate PR as v2 unless anyone objects.

JaviMerino · 2016-09-28T10:11:54Z

tests/eas/acceptance.py

+            switch_from = self.te.target.bl.bigs
+            switch_to   = self.te.target.bl.littles
+        else:
+            raise Exception("Invalid expected_switch_to")


ValueError is more specific than Exception

JaviMerino · 2016-09-28T10:19:54Z

tests/eas/acceptance.py

+            raise Exception("Invalid expected_switch_to")
+
+        expected_time = (self.get_start_time(experiment) +
+                         WORKLOAD_DURATION_S * phases)


I know we are almost never consistent with this part of pep8, but it recommends breaking line before the + and removing the spaces around the * because it takes higher preference

expected_time = (self.get_start_time(experiment) + WORKLOAD_DURATION_S*phases)

https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator
https://www.python.org/dev/peps/pep-0008/#other-recommendations

Ah, I didn't know about that. Will change.

JaviMerino · 2016-09-28T14:01:07Z

While you are at it, you could fix "targt" in the log output in libs/utils/executor.py, line 112

JaviMerino · 2016-09-28T14:13:11Z

tests/eas/acceptance.py

-            background=False)
-        cls.env.ftrace.stop()
-        trace = cls.env.ftrace.get_trace(cls.trace_file)
+    conf_basename = "wake_migration.config"


You forgot to git add tests/eas/wake_migration.config

JaviMerino · 2016-09-28T14:14:12Z

I've tried running this, but it explodes in a ball of flames. Some of the errors that I see, other than what I have pointed out in the review:

acceptance.experiment_test ... ERROR

======================================================================
ERROR: test suite for <class 'acceptance.EasTest'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/nose/suite.py", line 208, in run
    self.setUp()
  File "/usr/lib/python2.7/dist-packages/nose/suite.py", line 291, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/python2.7/dist-packages/nose/suite.py", line 314, in setupContext
    try_run(context, names)
  File "/usr/lib/python2.7/dist-packages/nose/util.py", line 471, in try_run
    return func()
  File "/vagrant/tests/eas/acceptance.py", line 66, in setUpClass
    cls.conf_basename)
AttributeError: type object 'EasTest' has no attribute 'conf_basename'

======================================================================
ERROR: test suite for <class 'acceptance.ForkMigration'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/nose/suite.py", line 208, in run
    self.setUp()
  File "/usr/lib/python2.7/dist-packages/nose/suite.py", line 291, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/python2.7/dist-packages/nose/suite.py", line 314, in setupContext
    try_run(context, names)
  File "/usr/lib/python2.7/dist-packages/nose/util.py", line 471, in try_run
    return func()
  File "/vagrant/tests/eas/acceptance.py", line 68, in setUpClass
    super(EasTest, cls)._init(conf_file, *args, **kwargs)
  File "/home/vagrant/lisa/libs/utils/test.py", line 53, in _init
    cls._runExperiments()
  File "/home/vagrant/lisa/libs/utils/test.py", line 72, in _runExperiments
    cls.executor.run()
  File "/home/vagrant/lisa/libs/utils/executor.py", line 154, in run
    self._wload_run(exp_idx, experiment)
  File "/home/vagrant/lisa/libs/utils/executor.py", line 526, in _wload_run
    self.te.emeter.report(experiment['out_dir'])
  File "/home/vagrant/lisa/libs/utils/energy.py", line 206, in report
    nrg_total = nrg[label]['total']
KeyError: 'a57'
-------------------- >> begin captured logging << --------------------

Why is it even looking at energy?

======================================================================
ERROR: acceptance.experiment_test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/usr/local/lib/python2.7/dist-packages/wrapt/decorators.py", line 282, in _wrapper
    target_wrapped = args[0]
IndexError: tuple index out of range

bjackman · 2016-09-29T09:13:46Z

@JaviMerino Re: hwmon error: It looks at hwmon because it's in the default Devlib modules. I thought that that error was due to the way I'd set up my kernel (the hwmon module doesn't produce any errors but it produces empty data, hence they key errors) and disabled hwmon in my target.config. My plan was in a separate PR to a) make hwmon disabled by default (only enabled on tests that need it) and b) make it fail more gracefully when the board doesn't support it as expected. Since you're seeing the error too, I'll take a look at it before submitting v2 of this.

Re: missing wake_migration.config, I noticed that shortly after submitting the PR :$ will be back in v2.

Re: s/targt/target/: will do.

bjackman · 2016-09-29T16:45:49Z

The HWMon issue is present in master too, but it doesn't manifest because the existing acceptance tests don't call report() (whereas the Executor class does this after running the workload).

I'm going to disable hwmon in the test configs (unfortunately I can't see a way to disable using energy meters in general, only hwmon specifically), and raise a separate issue to figure out why HWMon isn't working. Disabling it in the test configs is reasonably sensible anyway since they don't need energy readings, having hwmon just slows things down.

bjackman · 2016-10-03T13:09:54Z

superseded by v2: #202

Brendan Jackman added 15 commits September 27, 2016 17:55

libs/utils/executor: Remove irrelevant log messages

e992e83

libs/utils/executor: Default to 1 iteration if not specified

10cf60d

libs/utils/executor: Rework a 'for' to use dict.items

be11662

tests/eas/acceptance: Remove unused code

2721bfb

tests/eas/acceptance: Remove unused import

c4279d0

tests/eas/acceptance: Cleanup imports

2a8c402

Re-order imports according to PEP8 [1]. Also remove duplicate `import json`. [1] https://www.python.org/dev/peps/pep-0008/#imports

tests/eas/acceptance: Use min in OffloadMigrationAndIdlePull

fa1b1f9

tests/eas/acceptance: Add EasTest API

d073c23

This adds a class EasTest that inherits from LisaTest to provide a common set of functions for EAS acceptance tests.

tests/eas/acceptance: Port ForkMigration to EasTest

3efc847

tests/eas/acceptance: Port SmallTaskPacking to LisaTest

f3800cb

tests/eas/acceptance: Port OffloadMigrationAndIdlePull to LisaTest

7d4cf5b

tests/eas/acceptance: Port WakeMigration test to use LisaTest

49ba579

tests/eas/acceptance: Remove unused code

3a67afb

Now that all the EAS acceptance tests have been ported to use the EasTest class this code is no longer needed

bjackman assigned derkling and mdigiorgio Sep 27, 2016

derkling suggested changes Sep 27, 2016

View reviewed changes

derkling added this to the 16.10 milestone Sep 27, 2016

derkling added the enhancement label Sep 27, 2016

derkling mentioned this pull request Sep 27, 2016

tests/eas/capacity_capping: Add a capacity capping test #195

Merged

bjackman mentioned this pull request Sep 28, 2016

Some refactoring/enhancements to the Executor class #189

Closed

JaviMerino reviewed Sep 28, 2016

View reviewed changes

bjackman commented Sep 28, 2016

View reviewed changes

JaviMerino reviewed Sep 28, 2016

View reviewed changes

JaviMerino mentioned this pull request Sep 29, 2016

test/eas/acceptance: documentation fixes #199

Merged

bjackman mentioned this pull request Oct 3, 2016

Rework EAS acceptance tests to inherit from LisaTest v2 #202

Merged

bjackman closed this Oct 3, 2016

Rework EAS acceptance tests to inherit from LisaTest #197

Rework EAS acceptance tests to inherit from LisaTest #197

Conversation

bjackman commented Sep 27, 2016

derkling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjackman Sep 28, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjackman Sep 28, 2016 • edited

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

derkling Sep 27, 2016 • edited

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

bjackman Sep 28, 2016 • edited

Choose a reason for hiding this comment

bjackman left a comment

Choose a reason for hiding this comment

bjackman Sep 28, 2016 • edited

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

bjackman Sep 28, 2016 • edited

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

bjackman commented Sep 28, 2016

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

JaviMerino commented Sep 28, 2016

Choose a reason for hiding this comment

JaviMerino commented Sep 28, 2016 • edited

bjackman commented Sep 29, 2016

bjackman commented Sep 29, 2016

bjackman commented Oct 3, 2016

bjackman Sep 28, 2016 •

edited

bjackman Sep 28, 2016 •

edited

derkling Sep 27, 2016 •

edited

bjackman Sep 28, 2016 •

edited

bjackman Sep 28, 2016 •

edited

bjackman Sep 28, 2016 •

edited

JaviMerino commented Sep 28, 2016 •

edited