[Fuego] [PATCH] common: add new criteria operator bt
Qiu, Tingting
qiutt at cn.fujitsu.com
Fri Sep 28 02:20:51 UTC 2018
Hi, Tim
Thank you for your amendment. Indeed, ',' is better.
> OK - this is a lot of extra work and overhead (including the op for every
> criteria reference, for every measure of every benchmark), to support
> just a single operator ('bt') which might not be used that much.
>
> It appears to me that this is only used one place in the make_measure_plot
> code (the 'if entry.op == '"bt"'). Could you detect this automatically if
> the ref value had a ',' in it? This would save having to extend the
> flat_plot_data
> by an extra field, which is desirable.
You are right, we can use 'entry.ref' instead of 'entry.op' here,
but I also considered using 'entry.op' or 'entry.ref' of the if code.
Finally, 'entry.op' is selected because:
1.The 'entry.op' is easier to understand.
2.Maybe there will be other special operates in future. The 'entry.op' is easier to extend.
> Can you please consider the change I recommend above, as well as
> test this code, which I just pushed to master, to see if anything broke.
>
> I also did a refactoring patch on top of this, for check_measure, to
> make it a bit simpler.
>
> Please take a look.
I tested it briefly. It works very well.
Regards,
Qiu Tingting
> -----Original Message-----
> From: Tim.Bird at sony.com [mailto:Tim.Bird at sony.com]
> Sent: Friday, September 28, 2018 3:35 AM
> To: Qiu, Tingting/仇 婷婷; fuego at lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH] common: add new criteria operator bt
>
> > -----Original Message-----
> > From: Qiu Tingting
> >
> > Add new criteria operator bt to support that criteria check result is pass
> when
> > ouput value is between min value and max value.
> > e.g. of criteria file
> > "reference":{
> > "value":"7.5-10.5",
>
> '-' is not a good range delimiter here, because it could be used as part
> of a number value (for a negative number).
> Possibly ',' would work, but this patch is quite involved and I didn't have
> time to test it.
>
> See more explanation below.
>
> > "operator":"bt"
> > }
> >
> > Signed-off-by: Qiu Tingting <qiutt at cn.fujitsu.com>
> > ---
> > engine/scripts/parser/common.py | 20 +++++---
> > engine/scripts/parser/prepare_chart_data.py | 79
> > +++++++++++++++++++++--------
> > 2 files changed, 69 insertions(+), 30 deletions(-)
> >
> > diff --git a/engine/scripts/parser/common.py
> > b/engine/scripts/parser/common.py
> > index 9f3a8bb..5bea7f6 100644
> > --- a/engine/scripts/parser/common.py
> > +++ b/engine/scripts/parser/common.py
> > @@ -158,17 +158,19 @@ def check_measure(tguid, criteria_data, measure):
> > return 'SKIP'
> >
> > if reference['operator'] == 'lt':
> > - result = measure['measure'] < reference["value"]
> > + result = measure['measure'] < float(reference["value"])
> > elif reference['operator'] == 'le':
> > - result = measure['measure'] <= reference["value"]
> > + result = measure['measure'] <= float(reference["value"])
> > elif reference['operator'] == 'gt':
> > - result = measure['measure'] > reference["value"]
> > + result = measure['measure'] > float(reference["value"])
> > elif reference['operator'] == 'ge':
> > - result = measure['measure'] >= reference["value"]
> > + result = measure['measure'] >= float(reference["value"])
> > elif reference['operator'] == 'eq':
> > - result = measure['measure'] == reference["value"]
> > + result = measure['measure'] == float(reference["value"])
> > elif reference['operator'] == 'ne':
> > - result = measure['measure'] != reference["value"]
> > + result = measure['measure'] != float(reference["value"])
> > + elif reference['operator'] == 'bt':
> > + result = measure['measure'] >=
> float(reference["value"].split('-')[0])
> > and measure['measure'] <= float(reference["value"].split('-')[1])
> splitting on '-' I problematical, because it will cause problems with negative
> numbers.
>
> For example, this code will fail with the range "-7--3"
>
> I changed the range delimiter to ',' so the above could be expressed as:
> "-7,-3"
>
> > status = 'PASS' if result else 'FAIL'
> >
> > dprint(" result=%s" % result)
> > @@ -283,7 +285,7 @@ def convert_reference_log_to_criteria(filename):
> > old_measure_id, operation = crit_str.split("|")
> > ts_name, tc_name, measure_name =
> split_old_id(old_measure_id)
> > tguid = "%s.%s.%s" % (ts_name, tc_name, measure_name)
> > - value = float(lines[i+1])
> > + value = lines[i+1]
> >
> > reference = {"value":value, "operator": operation.strip()}
> > crit = {"tguid":tguid, "reference": reference }
> > @@ -568,15 +570,17 @@ def process(results={}):
> > update_results_json(test_logdir, TESTDIR, REF_JSON)
> >
> > ref_map = {}
> > + op_map = {}
> > for crit in criteria_data["criteria"]:
> > # not all criteria are measure reference thresholds
> > try:
> > ref_map[crit["tguid"]] = crit["reference"]["value"]
> > + op_map[crit["tguid"]] = crit["reference"]["operator"]
> > except:
> > # ignore other data
> > pass
> >
> > - data_lines = store_flat_results(test_logdir, run_data, ref_map)
> > + data_lines = store_flat_results(test_logdir, run_data, ref_map,
> op_map)
> > make_chart_data(test_logdir, TESTDIR, CHART_CONFIG_JSON, data_lines)
> >
> > status = run_data.get('status', 'FAIL')
> > diff --git a/engine/scripts/parser/prepare_chart_data.py
> > b/engine/scripts/parser/prepare_chart_data.py
> > index 4bf23cd..87af7fa 100644
> > --- a/engine/scripts/parser/prepare_chart_data.py
> > +++ b/engine/scripts/parser/prepare_chart_data.py
> > @@ -66,9 +66,10 @@ class flat_entry:
> > self.tguid = \
> > self.test_set = \
> > self.ref = \
> > + self.op = \
> > self.result = "ERROR-undefined"
> > parts = line.split()
> > - if len(parts) != 9:
> > + if len(parts) < 9:
> > print("Error reading line '%s' in prepare_chart_data.py" %
> line)
> > print("Possible data corruption in flat_plot_data.txt")
> >
> > @@ -81,6 +82,7 @@ class flat_entry:
> > self.kernel = parts[5]
> > self.tguid = parts[6]
> > self.ref = parts[7]
> > + self.op = parts[9]
> > self.result = parts[8]
> >
> > # break apart tguid
> > @@ -103,10 +105,10 @@ class flat_entry:
> > pass
> >
> > def __str__(self):
> > - return "%s %s %s %s %s %s %s %s\n" % \
> > + return "%s %s %s %s %s %s %s %s %s\n" % \
> > (self.board, self.testname, self.spec,
> self.build_number,
> > self.timestamp, self.kernel, self.tguid, self.ref,
> > - self.result)
> > + self.result, self.op)
> >
> > def get_chart_config(TESTDIR, chart_config_filename):
> > try:
> > @@ -146,10 +148,12 @@ def reread_run_json_files(TESTDIR):
> > # and possibly FUEGO_CRITERIA_JSON_PATH)
> > criteria_data = load_criteria()
> > ref_map = {}
> > + op_map = {}
> > for crit in criteria_data["criteria"]:
> > # not all criteria are measure reference thresholds
> > try:
> > ref_map[crit["tguid"]] = crit["reference"]["value"]
> > + op_map[crit["tguid"]] = crit["reference"]["operator"]
> > except:
> > # ignore other data
> > pass
> > @@ -162,7 +166,7 @@ def reread_run_json_files(TESTDIR):
> > # read run_data
> > with open(run_filename) as run_file:
> > run_data = json.load(run_file,
> > object_pairs_hook=collections.OrderedDict)
> > - store_flat_results(test_logdir, run_data, ref_map)
> > + store_flat_results(test_logdir, run_data, ref_map, op_map)
> >
> > # Store tguid results in a flat list, space-separated
> > # this is stored in a file called flat_plot_data.txt in the test directory
> > @@ -173,7 +177,7 @@ def reread_run_json_files(TESTDIR):
> > #
> > # returns the list of lines in the updated flat results file
> >
> > -def store_flat_results(test_logdir, run_data, ref_map):
> > +def store_flat_results(test_logdir, run_data, ref_map, op_map):
> > # test log dir is up one directory
> >
> > plot_data_filename=test_logdir+"/flat_plot_data.txt"
> > @@ -212,7 +216,7 @@ def store_flat_results(test_logdir, run_data,
> > ref_map):
> > return
> >
> > dprint("ref_map=%s" % ref_map)
> > -
> > + dprint("op_map=%s" % op_map)
> > for test_set in test_sets:
> > ts_name = test_set["name"]
> > test_cases = test_set["test_cases"]
> > @@ -245,11 +249,13 @@ def store_flat_results(test_logdir, run_data,
> > ref_map):
> > except:
> > try:
> > m_ref = ref_map[tguid]
> > + m_op = op_map[tguid]
> > except:
> > m_ref = 0
> > - line = "%s %s %s %s %s %s %s %s %s\n" % \
> > + m_op = "none"
> > + line = "%s %s %s %s %s %s %s %s %s %s\n" % \
>
> OK - this is a lot of extra work and overhead (including the op for every
> criteria reference, for every measure of every benchmark), to support
> just a single operator ('bt') which might not be used that much.
>
> It appears to me that this is only used one place in the make_measure_plot
> code (the 'if entry.op == '"bt"'). Could you detect this automatically if
> the ref value had a ',' in it? This would save having to extend the
> flat_plot_data
> by an extra field, which is desirable.
>
> > (board, test_name, test_spec, build_number,
> timestamp,
> > - kernel, tguid, m_ref, m_result)
> > + kernel, tguid, m_ref, m_result, m_op)
> > plot_file.write(line)
> > data_lines.append(line)
> >
> > @@ -319,20 +325,49 @@ def make_measure_plots(test_name,
> > chart_config, entries):
> >
> > ref_series_key = series_key + "-ref"
> >
> > - dprint("ref_series_key=%s" % ref_series_key)
> > -
> > - if ref_series_key not in series_map:
> > - dprint("making a new series for '%s'" % ref_series_key)
> > - series_map[ref_series_key] = { "label": ref_series_key,
> > - "data": [],
> > - "points": {"symbol": "cross"} }
> > - # add a plot point for this ref entry
> > - try:
> > - value = float(entry.ref)
> > - except:
> > - value = 0
> > - point = [entry.build_number, value]
> > - series_map[ref_series_key]["data"].append(point)
> > + if entry.op == "bt":
> Maybe detect this case with 'if ',' in entry.ref:' instead of adding
> a whole field to flat_plot_data.txt
>
> > + ref_min_series_key = ref_series_key + "-min"
> > + dprint("ref_min_series_key=%s" % ref_min_series_key)
> > + if ref_min_series_key not in series_map:
> > + # add a plot point for this ref min entry
> > + dprint("making a new series for '%s'" %
> ref_min_series_key)
> > + series_map[ref_min_series_key] = { "label":
> ref_min_series_key,
> > + "data": [],
> > + "points": {"symbol": "cross"} }
> > + try:
> > + min_value = float(entry.ref.split('-')[0])
> This would require changing also.
>
> > + except:
> > + min_value = 0
> > + point = [entry.build_number, min_value]
> > + series_map[ref_min_series_key]["data"].append(point)
> > + try:
> > + max_value = float(entry.ref.split('-')[1])
> This would require changing also.
>
> > + except:
> > + max_value = None
> > + ref_max_series_key = ref_series_key + "-max"
> > + dprint("ref_max_series_key=%s" % ref_max_series_key)
> > + if ref_max_series_key not in series_map:
> > + # add a plot point for this ref max entry
> > + dprint("making a new series for '%s'" %
> ref_max_series_key)
> > + series_map[ref_max_series_key] = { "label":
> > ref_max_series_key,
> > + "data": [],
> > + "points": {"symbol": "cross"} }
> > + point = [entry.build_number, max_value]
> > + series_map[ref_max_series_key]["data"].append(point)
> > + else:
> > + dprint("ref_series_key=%s" % ref_series_key)
> > + if ref_series_key not in series_map:
> > + # add a plot point for this ref entry
> > + dprint("making a new series for '%s'" %
> ref_series_key)
> > + series_map[ref_series_key] = { "label":
> ref_series_key,
> > + "data": [],
> > + "points": {"symbol": "cross"} }
> > + try:
> > + value = float(entry.ref)
> > + except:
> > + value = 0
> > + point = [entry.build_number, value]
> > + series_map[ref_series_key]["data"].append(point)
> >
> > flot_data = series_map.values()
> > flot_data.sort(key=itemgetter('label'))
> > --
> > 2.7.4
>
> OK - this is horrible of me, but I'm short on time. I applied the patch, and
> changed the delimiter to ','. But I haven't had time to test it.
>
> Can you please consider the change I recommend above, as well as
> test this code, which I just pushed to master, to see if anything broke.
>
> I also did a refactoring patch on top of this, for check_measure, to
> make it a bit simpler.
>
> Please take a look.
>
> Thanks,
> -- Tim
>
>
More information about the Fuego
mailing list