Move thermal code out of trapp/run.py

[sinkap]

We should think of moving functions like plot_all_freqs plot_freq_hists to a specific module or have another class that inherits trappy.Run which is thermal specific. We can even have such code in bart.thermal and move all subsystem specific code to their respective subsystem in BART.

Please share your thoughts on this as well? Considering existing dependencies and backward compatibility.

[sinkap]

I am going to avoid the reformatting of the documentation of the trappy.plot_utils module. We can discuss where would be good place for this thermal specific plot_utils and also the re-write using trappy.plotter. I will also add a warning in the documentation so that we avoid building any new API dependencies

[JaviMerino]

:+1: don't update the plot_utils documentation. It should be on its way out.

[sinkap]

@JaviMerino @bjackman I am working on making the tooling more accessible and having the thermal code is really confusing. I know there might be some teams in ARM that still rely on the some of the notebooks, but I plan to remove it from head and bump up the version to 6. So if these libraries are needed, the teams can rely on version 5.10 What do you guys think?

[bjackman]

I think it's probably OK. Will double check with colleagues and get back to you.

[bjackman]

Can you clarify which code you want to remove?

[sinkap]

This is from the time TRAPpy was actually called cr2 (compare runs). I will be more useful to have these modules in BART as thermal behaviours and the plots in LISA. Having these in the root directory of trappy is quite confusing for new users

compare_runs.py 
cpu_power.py    
devfreq_power.py
idle.py 
pid_controller.py   
plot_utils.py
thermal.py  

[JaviMerino]

True, but that's not a good reason to fork the project. Do you really just want to fork it because there are some files that are confusing to new users?

[sinkap]

Not confusing new users should be a good enough reason in my opinion.

We had always acknowledged that these are legacy artefacts and would need clean-up some day. I find it confusing to demo the tools to new users when there is thermal code in the root import namespace. Not only does this build new dependencies, it makes the job of a future cleanup harder.

@JaviMerino do you know of users that are actively using this right now?

[bobbyb-arm]

There is discussion at ARM about this. From my perspective, it'd be useful to have a little time while we work out how we can migrate users of this functionality. I'd also rather not have a fork.

[sinkap]

Would you be okay if we add a deprecation warning as a first step in the next release?

[sinkap]

Adding @raw-bin for posterity.

[raw-bin]

Why is the proposal here being termed a fork ? Is that because @sinkap is going to add trappy to an org internal repo (which may evolve independently) ? IMO that's inevitable. If folk are truly invested in contributing back that will happen anyway. We should make it be known that we would like contribs back and move on. @sinkap - you're basically after a refactor of some bits yes ? Nothing being dropped or cut out ? For the rest: Shouldn't that just be a part of a planned refactoring exercise which is then (hopefully) advertised as a part of an upcoming release regime ? That way interested parties know there's a change coming and can chime in if they have a problem and from my very high level view, the code quality improves. Else we'll perpetually accumulate cruft, no ?

[derkling]

FWIW, in LISA we now have an (hopefully) properly defined interface to easily add "analysis namespaces". What I envision as a possible solution is to migrate the existing thermal analysis code into a new thermal_analysis.py source (or more if better suited) in this folder: https://github.com/ARM-software/lisa/tree/master/libs/utils/analysis

This will make available, to people willing to write notebooks or tests, a set of APIs to:

• get existing/new dataframes, via the implementation of "_dfg_*" methods, e.g.: https://github.com/ARM-software/lisa/blob/master/libs/utils/analysis/latency_analysis.py#L51
• generate pre-defined static plots which collects and correlate useful information, e.g.: https://github.com/ARM-software/lisa/blob/master/libs/utils/analysis/latency_analysis.py#L154

as well as reference notebooks, as the ones you find here: https://github.com/ARM-software/lisa/tree/master/ipynb/examples/trace_analysis and tests using these APIs, like the ones you find here: https://github.com/ARM-software/lisa/tree/master/tests

Could this be a reasonable mid-/long-term goal?

[bobbyb-arm]

@sinkap - a warning message is fine from my perspective. @derkling - Something in that direction sounds great to me. The ideal is an example notebook built on top of BART/LISA/trappy that has the essential visual properties of the legacy notebooks. (There's already some statistical verification in BART.) Note we'll also have to work with teams to ensure customer training documentation is updated. (E.g. https://goo.gl/9o6hDK)