[Draft] Enable Auto Table Generation#1475
[Draft] Enable Auto Table Generation#1475rfbgo wants to merge 1 commit intoGoogleCloudPlatform:developfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic generation of result tables when no table configuration is provided. It adds a new 'experiment_block_name' variable, updates the table schema to make columns optional, and implements logic to dynamically populate default columns with identity information and figures of merit. Feedback suggests moving the dynamic column generation logic out of the per-row extraction method to improve performance, relocating the 're' import to the top of the file to comply with PEP 8, and using 'any()' for more concise column existence checks.
| if self._default_columns: | ||
| # Add identity columns if not present | ||
| for var in ["application_name", "workload_name", "experiment_name"]: | ||
| if not any(c.name == var for c in self.columns): | ||
| col_conf = {"name": var, "expression": f"{{{var}}}"} | ||
| self.columns.append(ResultsColumn(col_conf)) | ||
|
|
||
| # Add variables from experiment template name | ||
| unexpanded_exp_template_name = app_inst.variables.get("experiment_template_name") | ||
| if unexpanded_exp_template_name: | ||
| import re | ||
| vars_in_name = re.findall(r"\{([^}]+)\}", unexpanded_exp_template_name) | ||
| for var in vars_in_name: | ||
| if not any(c.name == var for c in self.columns): | ||
| col_conf = {"name": var, "expression": f"{{{var}}}"} | ||
| self.columns.append(ResultsColumn(col_conf)) | ||
|
|
||
| # Add FOM columns | ||
| results = app_inst.result | ||
| for context in results.contexts: | ||
| for fom in context["foms"]: | ||
| fom_name = fom["name"] | ||
| context_name = context["name"] | ||
| # check if we already have it | ||
| found = False | ||
| for c in self.columns: | ||
| if c.figure_of_merit == fom_name and c.figure_of_merit_context == context_name: | ||
| found = True | ||
| break | ||
| if not found: | ||
| col_conf = { | ||
| "name": fom_name, | ||
| "figure_of_merit": fom_name, | ||
| "figure_of_merit_context": context_name, | ||
| } | ||
| self.columns.append(ResultsColumn(col_conf)) | ||
|
|
There was a problem hiding this comment.
There are a few issues with this block for generating default columns:
-
Performance: This logic is inside extract_row, which is called for every row. This is inefficient as it repeatedly checks for existing columns. This logic should be moved to a place where it's executed only once per table, for instance, by refactoring ResultsTables.build_tables to first determine all columns and then extract rows.
-
Import location: The import of 're' is inside a method. According to PEP 8, imports should be at the top of the file.
-
Readability: The check for existing FOM columns (lines 253-258) can be made more concise using any(). This improves readability without being overly complex.
References
- According to PEP 8, imports should be at the top of the file. (link)
1 participant
Feature PoC