tests/eas fixes #526

valschneider · 2017-12-13T18:14:47Z

This PR brings some fixes to the expected_placement plotter in generic.py and also restores the old working directory for AndroidTarget as a temp fix to ARM-software/devlib#225

mdigiorgio

LGTM, just a small comment if you have time and will to do it

mdigiorgio · 2017-12-14T09:19:31Z

tests/eas/generic.py

@@ -29,8 +29,6 @@
 from unittest import SkipTest

 import matplotlib.pyplot as plt
-import numpy as np
-import pylab as pl
 import os


Since you are at it you could do a general cleanup of the imports, which means having something like:

import one import two # etc. from foo import bar # etc.

derkling

Some small changes... but overall LGTM ;-)

derkling · 2017-12-14T10:26:41Z

libs/utils/env.py

@@ -525,6 +525,7 @@ def _init_target(self, force = False):
            self.target = devlib.AndroidTarget(
                    platform = platform,
                    connection_settings = self.__connection_settings,
+                    working_directory = '/data/local/tmp/devlib-target',


While looking at this, I remembered we had already a 'workdir' configuration option, which is configurable via target.conf for example, which AFAIR was there exactly for that scope... however, perhaps due to some refactoring, it's now defined but never used 👎

You can see it's definition at line 224 above.

Can you please wire this up to this existing configuration option?

Mind that there is also a WORKING_DIR_DEFAULT, which I don't like so much.... we should probably instead have something like:

elif platform_type.lower() == 'android': # Workaround for ARM-software/devlib#225 self.workdir = conf.get('workdir', '/data/local/tmp/devlib-target') self._log.debug('Setup ANDROID target...') self.target = devlib.AndroidTarget(...) #... else: raise ValueError('Config error: not supported [platform] type {}'\ .format(platform_type)) self.workdir = self.target.working_directory

where this last statement is just to ensure to always have the two variables aligned.

In the example above... of course in the Android case you pass in self.workdir as working_directory in the AndroidTarget ctor ;-)

Devlib already defines default working directories for each of its target types - could I just drop the WORKING_DIR_DEFAULT thing ? That way I can just make self.workdir default to None and let devlib handle it.

derkling · 2017-12-14T10:28:08Z

tests/eas/generic.py

@@ -322,7 +319,7 @@ def plot_expected_util(self, experiment, util_df):
                prev = time

        figname = os.path.join(experiment.out_dir, 'expected_placement.png')
-        pl.savefig(figname, bbox_inches='tight')
+        fig.savefig(figname, bbox_inches='tight')


This should better be on a different patch... either by itself, or eventually squased into "tests/eas/generic: Close exp_placement figure" which seems another fix... in this last case please update the commit message too.

AFAIR this change allows us to drop the pylab import, hence why it's in this commit.

Well, in that case, since we already use pl.savefig all around in the analysis modules... perhaps removing the pylab module perhaps should be a self contained patch which does the same all around. Do you think that's possible? At least we keep consistency on how we save generated plots...

Ok, I'll keep that for another PR.

Removes a duplicate import and re-arranges the order of the imports.

It seems like closing unused figures isn't a default behaviour, and we are getting some warning about that.

tests/eas/ was missing an __init__.py, which meant the backend set in tests/__init__.py wouldn't be used. This commit also removes the fill_between() call that is used to draw the expected_placement figure, as it seems the desired behaviour is not supported with the Agg backend ("Unknown property step").

Seems like the use of `self.workdir` got thrown away at some point. This commit reconnects it to the devlib target constructor, and also gets rid of the `WORKING_DIR_DEFAULT` definition since devlib already defines default workdirs for its targets.

As mentionned in ARM-software/devlib#225, using /sdcard causes some issues with the HiKey960. As we haven't had any issues with using /data/local/tmp/ as a devlib working directory, we now the use this directory by default.

valschneider · 2017-12-14T11:13:55Z

Reinstored the use of pl.savefig in tests/generic.py
Reinstored the use of self.workdir in env.py and set the default for AndroidTarget to /data/local/tmp/devlib-target

derkling

One minor note... lemme know: if it does not make sense we can merge as it is ;-)

derkling · 2017-12-14T11:41:47Z

tests/eas/generic.py

@@ -306,7 +306,6 @@ def plot_expected_util(self, experiment, util_df):
                color = 'red'

            tdf.plot(ax=ax[cpu], drawstyle='steps-post', title="CPU{}".format(cpu), color=color)
-            ax[cpu].fill_between(tdf.index, tdf, 0, step='post', color=color)


Does it make sense to test and use the method if the backend supports it?
I guess this allows to get better pretty locking plots when we run tests from out desktop....

I had a quick look for that and it didn't seem very intuitive - that "step" parameter is part of the fill_between API so imho it should be supported by all of them. I have a post-it somewhere to remind me to look at that again sometime in the future...

What about just a simple try: except pass?

Not too hot on that but we could do it.

I guess this allows to get better pretty locking plots when we run tests from out desktop....

For that you'll also have to change the backend defined in tests/__init__.py

As per chat we had, seems that fixing this is not enough to allow the removal of the Agg backend.
So, let's got with this version.

valschneider requested a review from derkling December 13, 2017 18:14

mdigiorgio reviewed Dec 14, 2017

View reviewed changes

valschneider force-pushed the tests/fix-plot-placement branch from 11dd7bf to cefd9ef Compare December 14, 2017 09:34

derkling suggested changes Dec 14, 2017

View reviewed changes

Valentin Schneider added 6 commits December 14, 2017 10:50

tests/eas/generic: Clean up imports

bf06b93

Removes a duplicate import and re-arranges the order of the imports.

tests/eas/generic: Add expected_placement plot title

e89a45c

tests/eas/generic: Close exp_placement figure

2bb0324

It seems like closing unused figures isn't a default behaviour, and we are getting some warning about that.

utils/env: Reinstore old AndroidTarget working directory

c21eacc

As mentionned in ARM-software/devlib#225, using /sdcard causes some issues with the HiKey960. As we haven't had any issues with using /data/local/tmp/ as a devlib working directory, we now the use this directory by default.

valschneider force-pushed the tests/fix-plot-placement branch from cefd9ef to c21eacc Compare December 14, 2017 11:12

derkling reviewed Dec 14, 2017

View reviewed changes

derkling approved these changes Dec 14, 2017

View reviewed changes

derkling merged commit 9c78978 into ARM-software:master Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/eas fixes #526

tests/eas fixes #526

valschneider commented Dec 13, 2017

mdigiorgio left a comment

mdigiorgio Dec 14, 2017

valschneider Dec 14, 2017

derkling left a comment

derkling Dec 14, 2017

derkling Dec 14, 2017

valschneider Dec 14, 2017

derkling Dec 14, 2017

valschneider Dec 14, 2017

derkling Dec 14, 2017

valschneider Dec 14, 2017

valschneider commented Dec 14, 2017 •

edited

Loading

derkling left a comment

derkling Dec 14, 2017

valschneider Dec 14, 2017

derkling Dec 14, 2017

valschneider Dec 14, 2017 •

edited

Loading

derkling Dec 14, 2017

tests/eas fixes #526

tests/eas fixes #526

Conversation

valschneider commented Dec 13, 2017

mdigiorgio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derkling left a 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

valschneider commented Dec 14, 2017 • edited Loading

derkling left a 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

valschneider Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valschneider commented Dec 14, 2017 •

edited

Loading

valschneider Dec 14, 2017 •

edited

Loading