Collect instrumentation data from correct dir. #45
Conversation
|
It looks like this fix interacts oddly with |
|
results in: Traceback (most recent call last):
File "../bin/plot_krun_results", line 1643, in <module>
inset_xlimits=options.inset_xlimits)
File "../bin/plot_krun_results", line 361, in main
core_cycles, cycles_ylimits, inset_xlimits)
File "../bin/plot_krun_results", line 986, in draw_page
p_exec_charts[index].plot_stack()
File "../bin/plot_krun_results", line 523, in plot_stack
self.plot_wallclock_times()
File "../bin/plot_krun_results", line 587, in plot_wallclock_times
self._plot_steady_equivalent(self.wallclock_axis)
File "../bin/plot_krun_results", line 672, in _plot_steady_equivalent
if (self.segment_means[0] + self.segment_vars[0] >= lower_bound and
AttributeError: 'ProcessExecChart' object has no attribute 'segment_vars' |
|
And a related bug, the steady-equivalent plots don't quite work in the insets when the user passes in
|
bin/plot_krun_results
Outdated
| # then we expect the VM instrumentation files to be in a directory | ||
| # called `warmup_instr_data/` If there is more than one data named | ||
| # *_instr_data/ then we issue a warning to the user and chose one a | ||
| # random. |
There was a problem hiding this comment.
Would an easier approach be to have a --instr-dir switch on the command line? Then the user can put the instrumentation data in a directory named as they wish.
There was a problem hiding this comment.
It's possible. Since @fsfod is our main user at the moment, I'm happy to go with whatever he thinks is sensible. If we do it this way though, I'll probably split the other fixes into a separate PR.
There was a problem hiding this comment.
Maybe keep it automatic with but the command line an optional override for weird situations
There was a problem hiding this comment.
As you wish, but let's try to keep it as simple as possible.
There was a problem hiding this comment.
To be honest, I think we either need to do one or the other. It would be really baffling for a user who doesn't pass in the CLI option to suddenly find that we have picked up data from a directory when they didn't expect it. Any opinions @ltratt ?
There was a problem hiding this comment.
This does sound like a case where an explicit option is the least surprising thing to do.
|
OK, I have recorded the |
|
OK, the latest commits work for me, we now pass in |
|
Needs squashing BTW |
|
Should bin/warmup_stats not also be changed to support --instr-dir? |
|
That's a good point... |
|
OK, this works for me, and I've fixed another small bug. Please do check before we squash though! |
|
--instr-dir for both warmup_stats and plot_krun_results works for me. |
bin/plot_krun_results
Outdated
| print('No VM instrumentation data is available.') | ||
| else: | ||
| if not os.path.exists(options.instr_dir): | ||
| fatal_error('%s (VM instrumentation data directory) does not exist.') |
There was a problem hiding this comment.
missing format argument here?
bin/plot_krun_results
Outdated
| if not os.path.exists(options.instr_dir): | ||
| fatal_error('%s (VM instrumentation data directory) does not exist.') | ||
| elif not os.path.isdir(options.instr_dir): | ||
| fatal_error('%s (VM instrumentation data directory) is not a directory.') |
There was a problem hiding this comment.
missing format argument here as well
|
Ready to squash? |
|
yes you can squash |
This sets a directory containing VM instrumentation data. A parser for the data must be present in warmup.vm_instruments in order for the data to be charted.
Only useful for generating plots with VM instrumentation data.
Fixes a bug where the window size of a set of benchmarks was not stored if the user passed in a Krun JSON results file which included outliers.
|
OK, squashed! |
This fixes a bug (Issue #43) where VM instrumentation data could not be drawn with changepoints or outliers, due to the way we were guessing the name of the directory containing the instrumentation
data.
In the new code, we look for all files or directories named
*_instr_data, filter out any that are not directories, and filter out any that do not have a prefix common to the filename of the benchmarking data. i.e. if the input filename is:warmup_outliers_w200_changepoints.json.bz2then we would accept directores such aswarmup_instr_data/orwarmup_whatever_instr_data/If there is more than one candidate directory, we issue a warning to the user and pick one at random.
Example (with outliers):
test_issue43.pdf
Fixes #43