store aggregated results in separate folder #683

OVI3D0 · 2024-10-16T22:49:30Z

Description

Changes OSB's file storage so that aggregated test results are stored in their own folder, aggregated_results alongside the test_executions folder.

Issues Resolved

#661

Testing

New functionality includes testing

make it + make test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

gkamat · 2024-10-17T17:10:23Z

osbenchmark/metrics.py

+        doc = test_execution.as_dict()
+        aggregated_execution_path = paths.aggregated_results_root(self.cfg, test_execution_id=test_execution.test_execution_id)
+        io.ensure_dir(aggregated_execution_path)
+        aggregated_file = os.path.join(aggregated_execution_path, "test_execution.json")


May be better to call this aggregated_test_execution.json.

@gkamat @IanHoang done + I added some changes to metrics.py so OSB can locate the new aggregate folder if you guys wouldn't mind taking a look👍

Signed-off-by: Michael Oviedo <[email protected]>

IanHoang

Left a couple suggestions

IanHoang · 2024-10-24T20:33:48Z

osbenchmark/metrics.py

+    def format_dict(d):
+        if d:
+            items = sorted(d.items())
+            return ", ".join(["%s=%s" % (k, v) for k, v in items])
+        else:
+            return None


We should avoid repeating helper functions. This already exists in the list_test_executions() function. It'd be better to move it out of both so that both can use it

IanHoang · 2024-10-24T21:00:59Z

osbenchmark/metrics.py

+    aggregated_test_executions = []
+    for test_execution in test_execution_store(cfg).list_aggregations():
+        aggregated_test_executions.append([
+            test_execution.test_execution_id,
+            time.to_iso8601(test_execution.test_execution_timestamp),
+            test_execution.workload,
+            format_dict(test_execution.workload_params),
+            test_execution.test_procedure_name,
+            test_execution.provision_config_instance_name,
+            format_dict(test_execution.user_tags),
+            test_execution.workload_revision,
+            test_execution.provision_config_revision])
+
+    if len(aggregated_test_executions) > 0:
+        console.println("\nRecent aggregated test executions:\n")
+        console.println(tabulate.tabulate(
+            aggregated_test_executions,
+            headers=[
+                "TestExecution ID",
+                "TestExecution Timestamp",
+                "Workload",
+                "Workload Parameters",
+                "TestProcedure",
+                "ProvisionConfigInstance",
+                "User Tags",
+                "workload Revision",
+                "Provision Config Revision"
+                ]))
+    else:
+        console.println("")
+        console.println("No recent aggregate tests found.")


This works but a lot of this is duplicated from list_test_executions(). How do you feel about creating a separate helper method and just passing in the methods like the following pseudo code"

def list_test_executions(cfg): list_test_helper(test_execution_store(cfg).list_aggregations(), "test executions") def list_aggregated_test_results(cfg): list_test_helper(test_execution_store(cfg).list_aggregations(), "aggregation tests") def list_test_helper(test_store_method, test_execution_type): def format_dict(d): if d: items = sorted(d.items()) return ", ".join(["%s=%s" % (k, v) for k, v in items]) else: return None test_executions = [] listed_test_executions = test_store_method for test_execution in listed_test_executions: ... console.println(f"No recent {test_execution_type} found.") ...

By putting boiler plate code in list_test_helper(), we will avoid repeating code and slim down both list_test_executions(cfg) and list_aggregated_test_results(cfg).

Good point! Did this

IanHoang · 2024-10-24T21:06:03Z

osbenchmark/benchmark.py

@@ -697,6 +698,8 @@ def dispatch_list(cfg):
        test_execution_orchestrator.list_pipelines()
    elif what == "test_executions":
        metrics.list_test_executions(cfg)
+    elif what == "aggregated_results":


Since you've introduced a new method called list_aggregations(), how do you feel about using aggregations here as opposed to aggregated_results? Aggregations would be more simple for users to type out. But on second thought, aggregations might be confused with OpenSearch aggregations. I'm open to either

Personally, I think aggregated_results is more explicit and clear about what it's referring to. But we could make it so either aggregations or aggregated_results triggers the same function maybe?

aggregated_results is more explicit and we can go forth with that

IanHoang · 2024-10-25T13:37:49Z

osbenchmark/metrics.py

@@ -1275,7 +1275,7 @@ def results_store(cfg):
        return NoopResultsStore()


-def list_test_executions(cfg):
+def list_test_helper(store, title):


Nit: store_item might be more apt.

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 requested review from IanHoang, gkamat, beaioun, cgchinmay, rishabh6788 and VijayanB as code owners October 16, 2024 22:49

OVI3D0 force-pushed the aggregate-folder branch from d04f9f2 to 97a2a7a Compare October 16, 2024 22:55

gkamat approved these changes Oct 17, 2024

View reviewed changes

OVI3D0 added 2 commits October 23, 2024 18:44

store aggregated results in separate folder

9152dba

Signed-off-by: Michael Oviedo <[email protected]>

update aggregated test exe json file name

6306767

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 force-pushed the aggregate-folder branch 2 times, most recently from db9dcf3 to 20d6048 Compare October 23, 2024 19:35

update OSB to find aggregated test results

447b7d6

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 force-pushed the aggregate-folder branch from 20d6048 to 447b7d6 Compare October 23, 2024 19:40

OVI3D0 requested a review from gkamat October 23, 2024 21:33

add separate option to list aggregations

f8b8c82

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 force-pushed the aggregate-folder branch from 5a2e279 to f8b8c82 Compare October 24, 2024 19:33

change aggregations to aggregated_results

43a9cc5

Signed-off-by: Michael Oviedo <[email protected]>

IanHoang requested changes Oct 24, 2024

View reviewed changes

OVI3D0 force-pushed the aggregate-folder branch from 192ec70 to 41c7f18 Compare October 24, 2024 22:08

OVI3D0 requested a review from IanHoang October 24, 2024 22:34

IanHoang reviewed Oct 25, 2024

View reviewed changes

IanHoang approved these changes Oct 25, 2024

View reviewed changes

OVI3D0 force-pushed the aggregate-folder branch from 41c7f18 to 8000774 Compare October 25, 2024 17:03

abstract logic to list test executions + aggregated results

7156d47

Signed-off-by: Michael Oviedo <[email protected]>

OVI3D0 force-pushed the aggregate-folder branch from 8000774 to 7156d47 Compare October 25, 2024 17:04

IanHoang merged commit bf1debc into opensearch-project:main Oct 25, 2024
10 checks passed

OVI3D0 deleted the aggregate-folder branch October 28, 2024 16:43

OVI3D0 mentioned this pull request Nov 25, 2024

[DOCUMENTATION] Updated results from aggregate command #700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store aggregated results in separate folder #683

store aggregated results in separate folder #683

OVI3D0 commented Oct 16, 2024 •

edited

Loading

gkamat Oct 17, 2024

IanHoang Oct 23, 2024

OVI3D0 Oct 23, 2024

IanHoang left a comment

IanHoang Oct 24, 2024

IanHoang Oct 24, 2024

OVI3D0 Oct 24, 2024

IanHoang Oct 24, 2024

OVI3D0 Oct 24, 2024

IanHoang Oct 25, 2024

IanHoang Oct 25, 2024

OVI3D0 Oct 25, 2024

store aggregated results in separate folder #683

store aggregated results in separate folder #683

Conversation

OVI3D0 commented Oct 16, 2024 • edited Loading

Description

Issues Resolved

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanHoang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OVI3D0 commented Oct 16, 2024 •

edited

Loading