Skip to content
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.

Sign up for GitHub

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

store aggregated results in separate folder #683

Merged
merged 6 commits into from
Oct 25, 2024
+38 −9
Merged

store aggregated results in separate folder #683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9152dba
store aggregated results in separate folder
OVI3D0 Oct 16, 2024
6306767
update aggregated test exe json file name
OVI3D0 Oct 23, 2024
447b7d6
update OSB to find aggregated test results
OVI3D0 Oct 23, 2024
f8b8c82
add separate option to list aggregations
OVI3D0 Oct 24, 2024
43a9cc5
change aggregations to aggregated_results
OVI3D0 Oct 24, 2024
7156d47
abstract logic to list test executions + aggregated results
OVI3D0 Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion osbenchmark/benchmark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ def add_workload_source(subparser):
metavar="configuration",
help="The configuration for which Benchmark should show the available options. "
"Possible values are: telemetry, workloads, pipelines, test_executions, provision_config_instances, opensearch-plugins",
choices=["telemetry", "workloads", "pipelines", "test_executions", "provision_config_instances", "opensearch-plugins"])
choices=["telemetry", "workloads", "pipelines", "test_executions", "aggregations",
"provision_config_instances", "opensearch-plugins"])
list_parser.add_argument(
"--limit",
help="Limit the number of search results for recent test_executions (default: 10).",
Expand Down Expand Up @@ -697,6 +698,8 @@ def dispatch_list(cfg):
test_execution_orchestrator.list_pipelines()
elif what == "test_executions":
metrics.list_test_executions(cfg)
elif what == "aggregations":
metrics.list_aggregated_test_results(cfg)
elif what == "provision_config_instances":
provision_config.list_provision_config_instances(cfg)
elif what == "opensearch-plugins":
Expand Down
46 changes: 44 additions & 2 deletions osbenchmark/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,45 @@ def format_dict(d):
console.println("")
console.println("No recent test_executions found.")

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Did this


def create_test_execution(cfg, workload, test_procedure, workload_revision=None):
provision_config_instance = cfg.opts("builder", "provision_config_instance.names")
Expand Down Expand Up @@ -1567,10 +1606,13 @@ def _test_execution_file(self, test_execution_id=None, is_aggregated=False):

def list(self):
results = glob.glob(self._test_execution_file(test_execution_id="*"))
aggregated_results = glob.glob(self._test_execution_file(test_execution_id="*", is_aggregated=True))
all_test_executions = self._to_test_executions(results + aggregated_results)
all_test_executions = self._to_test_executions(results)
return all_test_executions[:self._max_results()]

def list_aggregations(self):
aggregated_results = glob.glob(self._test_execution_file(test_execution_id="*", is_aggregated=True))
return self._to_test_executions(aggregated_results)

def find_by_test_execution_id(self, test_execution_id):
is_aggregated = test_execution_id.startswith('aggregate')
test_execution_file = self._test_execution_file(test_execution_id=test_execution_id, is_aggregated=is_aggregated)
Expand Down
Loading