-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Publish recall as a kpi metric #581
Changes from 4 commits
397b31b
44e37e6
e478335
a1b9af0
626953f
d40921d
387276c
4d38ad4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,6 +156,16 @@ def publish(self): | |
metrics_table.extend(self._publish_error_rate(record, task)) | ||
self.add_warnings(warnings, record, task) | ||
|
||
for record in stats.correctness_metrics: | ||
task = record["task"] | ||
|
||
keys = record.keys() | ||
recall_keys_in_task_dict = "recall@1" in keys and "recall@k" in keys | ||
if recall_keys_in_task_dict and record["recall@1"] and record["recall@k"]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't check for record['recall@k"], if recall is 0, it will be skipped. just check only for keys |
||
res = self._publish_recall(record, task) | ||
if res: | ||
metrics_table.extend(res) | ||
|
||
self.write_results(metrics_table) | ||
|
||
if warnings: | ||
|
@@ -200,14 +210,26 @@ def _publish_service_time(self, values, task): | |
def _publish_processing_time(self, values, task): | ||
return self._publish_percentiles("processing time", task, values["processing_time"]) | ||
|
||
def _publish_percentiles(self, name, task, value): | ||
def _publish_recall(self, values, task): | ||
recall_k = values["recall@k"] | ||
recall_1 = values["recall@1"] | ||
|
||
try: | ||
return self._join( | ||
self._line("Mean recall@k", task, recall_k["mean"], "", lambda v: "%.2f" % v), | ||
self._line("Mean recall@1", task, recall_1["mean"], "", lambda v: "%.2f" % v) | ||
) | ||
except KeyError: | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to catch this? can you return from like 214 if keys doesn't exist? |
||
|
||
def _publish_percentiles(self, name, task, value, unit="ms"): | ||
lines = [] | ||
percentiles = self.display_percentiles.get(name, metrics.GlobalStatsCalculator.OTHER_PERCENTILES) | ||
|
||
if value: | ||
for percentile in metrics.percentiles_for_sample_size(sys.maxsize, percentiles_list=percentiles): | ||
percentile_value = value.get(metrics.encode_float_key(percentile)) | ||
a_line = self._line("%sth percentile %s" % (percentile, name), task, percentile_value, "ms", | ||
a_line = self._line("%sth percentile %s" % (percentile, name), task, percentile_value, unit, | ||
force=self.publish_all_percentile_values) | ||
self._append_non_empty(lines, a_line) | ||
return lines | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
# not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
|
@@ -46,7 +46,6 @@ | |
from osbenchmark.workload import WorkloadProcessorRegistry, load_workload, load_workload_plugins | ||
from osbenchmark.utils import convert, console, net | ||
from osbenchmark.worker_coordinator.errors import parse_error | ||
|
||
################################## | ||
# | ||
# Messages sent between worker_coordinators | ||
|
@@ -847,6 +846,50 @@ def __call__(self, raw_samples): | |
start = total_start | ||
final_sample_count = 0 | ||
for idx, sample in enumerate(raw_samples): | ||
self.logger.debug( | ||
"All sample meta data: [%s],[%s],[%s],[%s],[%s]", | ||
self.workload_meta_data, | ||
self.test_procedure_meta_data, | ||
sample.operation_meta_data, | ||
sample.task.meta_data, | ||
sample.request_meta_data, | ||
) | ||
|
||
# if request_meta_data exists then it will have {"success": true/false} as a parameter. | ||
if sample.request_meta_data and len(sample.request_meta_data) > 1: | ||
self.logger.debug("Found: %s", sample.request_meta_data) | ||
recall_metric_names = ["recall@k", "recall@1"] | ||
|
||
for recall_metric_name in recall_metric_names: | ||
if recall_metric_name in sample.request_meta_data: | ||
meta_data = self.merge( | ||
self.workload_meta_data, | ||
self.test_procedure_meta_data, | ||
sample.operation_meta_data, | ||
sample.task.meta_data, | ||
sample.request_meta_data, | ||
) | ||
|
||
self.logger.debug( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this to log? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this -- It's debug level so it shouldn't show up with normal logging settings. I was using it to figure out how samples worked, so this log has served its purpose and I'll remove it now. |
||
"Here are the sample stats: Task: %s, operation: %s, operation_type; %s, sample_type: %s", | ||
sample.task.name, | ||
sample.operation_name, | ||
sample.operation_type, | ||
sample.sample_type, | ||
) | ||
self.metrics_store.put_value_cluster_level( | ||
name=recall_metric_name, | ||
value=sample.request_meta_data[recall_metric_name], | ||
unit="", | ||
task=sample.task.name, | ||
operation=sample.operation_name, | ||
operation_type=sample.operation_type, | ||
sample_type=sample.sample_type, | ||
absolute_time=sample.absolute_time, | ||
relative_time=sample.relative_time, | ||
meta_data=meta_data, | ||
) | ||
|
||
if idx % self.downsample_factor == 0: | ||
final_sample_count += 1 | ||
meta_data = self.merge( | ||
|
@@ -895,7 +938,7 @@ def __call__(self, raw_samples): | |
end = time.perf_counter() | ||
self.logger.debug("Calculating throughput took [%f] seconds.", (end - start)) | ||
start = end | ||
for task, samples in aggregates.items(): | ||
for task, samples in aggregates.items(): # returns dict of task, and samples. | ||
meta_data = self.merge( | ||
self.workload_meta_data, | ||
self.test_procedure_meta_data, | ||
|
@@ -1190,6 +1233,7 @@ def __init__(self, start_timestamp, buffer_size=16384): | |
def add(self, task, client_id, sample_type, meta_data, absolute_time, request_start, latency, service_time, | ||
client_processing_time, processing_time, throughput, ops, ops_unit, time_period, percent_completed, | ||
dependent_timing=None): | ||
self.logger.debug("Logging with metadata: [%s]", meta_data) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this? |
||
try: | ||
self.q.put_nowait( | ||
Sample(client_id, absolute_time, request_start, self.start_timestamp, task, sample_type, meta_data, | ||
|
@@ -1369,7 +1413,7 @@ def calculate_task_throughput(self, task, current_samples, bucket_interval_secs) | |
self.task_stats[task] = ThroughputCalculator.TaskStats(bucket_interval=bucket_interval_secs, | ||
sample_type=first_sample.sample_type, | ||
start_time=first_sample.absolute_time - first_sample.time_period) | ||
current = self.task_stats[task] | ||
current = self.task_stats[task] # TaskStats object | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this comment here and as part of this PR? |
||
count = current.total_count | ||
last_sample = None | ||
for sample in current_samples: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove unrelated formatting changes from this PR?