pranavbhandari24 commented on code in PR #28671: URL: https://github.com/apache/beam/pull/28671#discussion_r1343110224
########## sdks/python/apache_beam/testing/analyzers/README.md: ########## @@ -35,16 +35,13 @@ update already created GitHub issue or ignore performance alert by not creating ## Config file structure -The config file defines the structure to run change point analysis on a given test. To add a test to the config file, +The yaml defines the structure to run change point analysis on a given test. To add a test config to the yaml file, please follow the below structure. -**NOTE**: The Change point analysis only supports reading the metric data from Big Query for now. +**NOTE**: The Change point analysis only supports reading the metric data from `BigQuery` only. Review Comment: nit: `only` is repeated, we can omit it from the end of the sentence ########## sdks/python/apache_beam/testing/analyzers/perf_analysis.py: ########## @@ -169,12 +182,11 @@ def run(config_file_path: Optional[str] = None) -> None: tests_config: Dict[str, Dict[str, Any]] = read_test_config(config_file_path) - big_query_metrics_fetcher = BigQueryMetricsFetcher() - for test_name, params in tests_config.items(): + for test_id, params in tests_config.items(): Review Comment: This can be done by adding `metric_name` as a parameter to `run_change_point_analysis` instead of accessing it directly in the method? I'll defer to your opinion ########## sdks/python/apache_beam/testing/analyzers/perf_analysis.py: ########## @@ -169,12 +182,11 @@ def run(config_file_path: Optional[str] = None) -> None: tests_config: Dict[str, Dict[str, Any]] = read_test_config(config_file_path) - big_query_metrics_fetcher = BigQueryMetricsFetcher() - for test_name, params in tests_config.items(): + for test_id, params in tests_config.items(): Review Comment: We had spoken about supporting multiple `metric_name` for a single `test_id`. These metric names will be provided as a list in the config. Shouldn't we check if `params['metric_name']` is a list and execute `run_change_point_analysis` for each metric_name? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
