Energy model-based tests, RFC v2

[bjackman]

See the commit messages for the first and fourth commits for more detail on what this PR aims to introduce. In short, this aims to allow automatically running tests for any given synthetic workload and platform.

This looks like an enormous PR, and it is... but a lot of the added lines are .ipynb noise or boilerplate in the provided platform descriptions. I've also added pretty verbose docstrings which bumps up the line count a lot.

I've siloed myself a bit on this so please highlight wherever I haven't made the high-level intention clear, It's important that someone can understand what this stuff is trying to do without reading 2k lines of inscrutable bjackman Python 😄 . Hopefully docstrings are a good starting point...

[bjackman]

Another example use: https://github.com/bjackman/lisa/blob/cie-test/tests/eas/load_tracking.py#L247

[bjackman]

Today I ran the "generic" tests I've added here and compared the energy estimation from EnergyModel with the energy measured with an ACME cape on Pixel and hwmon on Juno.

The only numerical analysis I did was to compare the estimated and measured energy for the entirety of each workload. There's a strong correlation (>95% coefficient) there.

As for finer grained predictions (i.e. predicting& measuring the variation in energy consumption during the course of the workload), I plotted energy measurements over time (using ACME) and compared visually with plots of energy predictions over time. They look to match nicely but like I say this wasn't very scientific.

Here's the notebook I used: https://github.com/bjackman/lisa/blob/energy-model-rfc-v2-evaluation/ipynb/Energy_Model_Eval.ipynb

[ionela-voinescu]

Awesome job! This is my first set of comments (mostly nit-picky and about how to make the information more clear in the test notebook). I'm still giving some thought to the EnergyModel class and I'll follow up with that later).

[ionela-voinescu]

This class also has some references to bl.littles and bl.bigs in _assert_switch that were not replaced. I can't seem to be able to post a comment on those lines :-|.

[bjackman]

Will take a look.

[ionela-voinescu]

I see this set to false in the other classes also. If I haven't missed anything all tests will be run on SMP also and none of the tests will be skipped. I suppose the result of this should be that some of the tests are skipped on SMP as you won't see some of these behaviours that are expected there (behaviours characteristic to big.LITTLE). Therefore, shouldn't some of these have skip_on_smp set to True?

[ionela-voinescu]

Shouldn't all task start on the big CPU only if the initial task utilisation was set to 1020 (that is if SET_INITIAL_TASK_UTIL)? Otherwise a failure of a task to start on the big CPU might not be a failure. As discussed it would be best if this test was removed or at least constrained.

[bjackman]

Yes. This function is just being moved from acceptance.py so I'll leave it here for now. None of the new tests should use it.

[ionela-voinescu]

I suppose you meant: ".. are shown as running on the last CPU they ran on."

[ionela-voinescu]

Can we have this in a meaningful (sorted) order as to be more clear? Also, as discussed, you could get rid of the power column or have it at the end for it to be more clear that it represents the total power.

[bjackman]

Yep. I think I'll remove the "power" column and sort the node columns by depth-first post-order.

[ionela-voinescu]

The 20% (1.2) value hard-coded here does not look good to me. Can we have it as a class variable as for negative_slack_allowed_pct?

[bjackman]

Yep, will do.

[ionela-voinescu]

I think these specific test cases should be in their own file and to keep generic.py hosting only the EnergyModelTest class.

[bjackman]

Why? In this case "generic" is supposed to mean "works on any platform".

[ionela-voinescu]

I thought generic meant more that it would hold the base EnergyModelTest class, which then can be extended in specific test classes. I find it more clear to have the EnergyModelTest class in its own file but it's just a preference.

[ionela-voinescu]

It would be more clear if these would print a success message when the test is passed as there is no output from it otherwise. The same for test_task_placement.

[bjackman]

True, will add this.

[ionela-voinescu]

While I understand why you decided not to print the data frames for this call and the ones below, I for one would like to have more information in these test notebooks as it gives me more confidence in the test. If you have some calls to some functions that do not return anything it might make people think that something is wrong and maybe the test is not doing what it's suppose to be doing. I think an option would be to print the traces and plots first and maybe have a debug section after that where the data frames are printed and the process is explained in a few sentences.

[bjackman]

The functions do return something, it's just assigned to a variable, I wouldn't expect to see any output from running that code. But you're right that this is unclear.

How about if we print the docstring for the function before calling it, then you basically get a detailed description of what's going on.

[ionela-voinescu]

Sorry! I did not make myself clear. I did not mean to say that the calls don't return anything but that you can't see anything being printed as a result of them being run. I suppose printing the docstrings could work but what I meant to say is that it would be nice to have a section where you can see more details on the test being run. Details on the workload, the data frames obtained, something that could represent the basis of a debugging process.

[bjackman]

I don't intend for this to be for debugging the tests, after all they are absolutely flawless in every single way. Rather it's intended as a starting point for looking at what element of the kernel behaviour was judged to be faulty (since when you have a failure from a lisa-test run it's pretty much "computer says no").

Basically the only thing I mean for this to show is a plot of the workload task residency and a plot of the energy estimations, so you can see which part of the workload run was judged to be mis-scheduled. I don't imagine users diving python in order to do that, but rather just taking a look and seeing "oh yeah this 5% task ran on a big CPU at t=3.5" then going back to the kernel to figure out why that happened.

Nonetheless I'll add some more english to explain what is going on here.

[ionela-voinescu]

It would be good to have more information about what the tests report and how the evaluation is done. As you say below "These tests make estimation of the energy efficiency of task placements.. " + "by.. ". It would also be nice to make clear the distinction between the estimated observed power consumption (estimated based on the energy model provided and the observed task placement decisions made) and the expected power consumption (estimated based on the energy model provided and the expected optimal task placements). This can be done either here at the beginning or further in the notebook where the estimated and expected data frames are obtained.

[bjackman]

True. I will try to clarify in the notebook.

[bjackman]

OK, thanks for the review. I've responded to the comments to remind myself what changes I need to make, and I'll come back once I have time to make the code changes.

[bjackman]

OK I've pushed a new version where I've tried to address your comments:

• Fixing the minor stuff
• I added some more text to the notebook to explain what's going on. Also some of the calls that you commented on actually weren't necessary so I removed them. Hopefully it's a bit more easy to understand (assuming people actually read my walls of text 😬)
• Fixed the order of the columns in power DataFrames (For juno you get [(1), (2), (1, 2), (0), (3), (4), (5), (0, 3, 4, 5)] i.e. big CPUs followed by big cluster, then LITTLE CPUs followed by LITTLE cluster).
• Fixed the remaining bl references in acceptance.py.

[ionela-voinescu]

Thank you very much for changes! The details in the notebook make a world of difference. They will be read!

[ionela-voinescu]

These are no longer printed (based on a commit merged a short while ago).

[ionela-voinescu]

.. is higher than the ideal energy plot..
Although a reword of it might work: "Where the values for the observed energy are higher than the ideal energy values..". The same for the following sentence. It's strange to say "where the plot is higher". Maybe a better wording would be "where the plot line is higher.. ".

[bjackman]

Good spot. Good point about "plot" vs "values" will reword this.

[ionela-voinescu]

Awesome! :D

[ionela-voinescu]

Isn't this the same as saying: LisaLogging.setup(level=logging.WARNING)?

[bjackman]

Yep, will change.

[ionela-voinescu]

"Test EAS for tasks alternating beetween 10% and 50% over 4 seconds". It would be good to specify the duration of the entire workload as for the time axis of the plot to be better understood. The same for the other classes. Some of them include this information but some of them do not.

[bjackman]

Updated

[credp]

This looks really nice :) I'm just using it now.

One comment as we just mentioned.

It would be pretty nice if when we get a fail like:
AssertionError: Estimated 984.663979795 bogo-Joules to run workload, expected 741.998222222
there was some indication what the 'best' task placement would have looked like - because an interested engineer is likely to load up the trace file and have a look to see what the placement was. It should be sufficient to just return the first one, as an example.

[derkling]

Ok, I can finally confirm that going thought the review of this code can brings great health benefits to everybody life! ❤️

Following are some comments on possible updates and small improvements, mostly on the documentation side, and few small little things on the code side.

Here is also a branch with some patches related to comments:
https://github.com/derkling/lisa/tree/energy-model-rfc-v2-review

Overall: great work! 👍 It's pretty ready for merge for me too! ;-)

[derkling]

nit-pick: we should add a comment for consistency

possible PATCH in the branch for that!

[derkling]

let's format one per line for consistency.

[derkling]

Perhaps we can make a better usage of spaces to have more aligned data... will send a possible reformatting patch.

possible PATCH in the branch for this and other minor comments on EM formatting.

[bjackman]

Sure. I'm a little bit awkward about doing this because for some reason my PEP8 religion forbids it but TBH it is clearer so let's apply it here.

[derkling]

s/eat/at/

[derkling]

Code it's already complex by its own... I would rather prefer to avoid args/kwargs when not really required.
Seems that here it's just to "write less code", but since this is just a subclass of EnergyModelNote, why not having:

def __init__(self, active_states=None, idle_states=None,
             cpu=None, children=None, name=None):

it requires one less jump when reading the code

[bjackman]

Sure, it seems clearer.

[derkling]

s/placments/placements/

[bjackman]

mm.. now I'm thinking about....

🎈 CI FOR SPELLING 🎈

[derkling]

Small optimisations but here we can easily do:

candidates = {}
excluded = []
for cpus in product(self.cpus, repeat=len(tasks)):
    ...
    util = tuple(util)
    if (any([u > 1024 for u in util]) or
        util in excluded):
        continue
    if not candidates:
        ...
        if not overutilized:
            ....
        else:
            excluded.append(util

[bjackman]

Nice, thanks!

[derkling]

This clobbers the awesome test output... should we better use debug?
Same for the DONE log at the end...

[derkling]

Thinking now, looking at the tests, could be useful to have a minimal sanity check on the proposed energy model data.
For example, energy in active states should always be monotonically decreasing... frequencies monotonically increasing... and than have some assert on top of these conditions.

[bjackman]

Sounds good.

I did also implement a method to print various observations about the energy model but didn't include it here. It's more for warnings: "this EM is technically valid but doesn't make sense" rather than "this is a totally invalid EM" though.

[derkling]

As a really general comment, all the following tests are quite dependant on the hardcoded values of the EM described before, which is fine until someone does not want to slightly change the EM definition to add yet another test.

Would not be more robust to express the tests in terms of EM data?

Thus, for example this 350 should be replaced by the max capacity defined by cluster_big's active states?

But I don't really care if you want to keep the test as it is now... perhaps just add a bold warning in the comment at the beginning to info the malicious user that he can hurt himself by changing only the EM parameters without updating the following tests.

[bjackman]

Yeah, this was a deliberate decision at the beginning of writing it, because I thought if I expressed it in terms of the data I'd probably make the exact same mistakes as I'd made in the code under test. So I wanted to work them out by hand then check them with my model.

However I agree that as the code grew it started to look like less of a smart idea! I'm going to keep it as it is for now, but add a comment. Probably if we need to extend these tests it will be faster to add new ones in terms of the data rather than go back and re-factor the hard coded stuff. Anyway, the risk of breaking things isn't such a sword of Damocles, the beauty of unit tests is that you can tell straight away :D