Address comments and add more unit tests

Author: pvk-developer

sdgym/benchmark.py

@@ -111,7 +111,7 @@
 SDV_SYNTHESIZERS = SDV_SINGLE_TABLE_SYNTHESIZERS + SDV_MULTI_TABLE_SYNTHESIZERS
 
 
-def _validate_inputs(output_filepath, detailed_results_folder, synthesizers, custom_synthesizers):
+def _validate_output_filepath_and_detailed_results_folder(output_filepath, detailed_results_folder):
     if output_filepath and os.path.exists(output_filepath):
         raise ValueError(
             f'{output_filepath} already exists. Please provide a file that does not already exist.'
@@ -123,15 +123,65 @@ def _validate_inputs(output_filepath, detailed_results_folder, synthesizers, cus
             'Please provide a folder that does not already exist.'
         )
 
-    duplicates = get_duplicates(synthesizers) if synthesizers else set()
-    if custom_synthesizers:
-        duplicates.update(get_duplicates(custom_synthesizers))
-    if len(duplicates) > 0:
+
+def _import_and_validate_synthesizers(synthesizers, custom_synthesizers, modality):
+    """Import user-provided synthesizer and validate modality and uniqueness.
+
+    This function takes lists of synthesizer, imports them as synthesizer classes,
+    and validates two conditions:
+        - Modality match – all synthesizers must match the expected `modality`.
+        A `ValueError` is raised if any synthesizer has a different modality
+        flag.
+
+        - Uniqueness – duplicate synthesizer across the two input lists
+        (`synthesizers` and `custom_synthesizers`) are not allowed. A
+        `ValueError` is raised if duplicates are found.
+
+    Args:
+        synthesizers (list | None):
+            A list of synthesizer strings or classes. May be ``None``, in which case it
+            is treated as an empty list.
+        custom_synthesizers (list | None):
+            A list of custom synthesizer.
+        modality (str):
+            The required modality that all synthesizers must match.
+
+    Returns:
+        list:
+            A list of synthesizer classes.
+
+    Raises:
+        ValueError:
+            If any synthesizer does not match the expected modality.
+        ValueError:
+            If duplicate synthesizer are found across the provided lists.
+    """
+    # Get list of synthesizer objects
+    synthesizers = synthesizers or []
+    custom_synthesizers = custom_synthesizers or []
+    resolved_synthesizers = get_synthesizers(synthesizers + custom_synthesizers)
+    mismatched = [
+        synth['synthesizer']
+        for synth in resolved_synthesizers
+        if synth['synthesizer']._MODALITY_FLAG != modality
+    ]
+    if mismatched:
+        raise ValueError(
+            f"Synthesizers must be of modality '{modality}'. "
+            "Found this synthesizers that don't match: "
+            f'{", ".join([type(synth).__name__ for synth in mismatched])}'
+        )
+
+    # Check duplicate input values
+    duplicates = get_duplicates(synthesizers + custom_synthesizers)
+    if duplicates:
         raise ValueError(
             'Synthesizers must be unique. Please remove repeated values in the `synthesizers` '
             'and `custom_synthesizers` parameters.'
         )
 
+    return resolved_synthesizers
+
 
 def _create_detailed_results_directory(detailed_results_folder):
     if detailed_results_folder and not is_s3_path(detailed_results_folder):
@@ -276,15 +326,9 @@ def _generate_job_args_list(
     compute_diagnostic_score,
     compute_privacy_score,
     synthesizers,
-    custom_synthesizers,
     s3_client,
     modality,
 ):
-    # Get list of synthesizer objects
-    synthesizers = [] if synthesizers is None else synthesizers
-    custom_synthesizers = [] if custom_synthesizers is None else custom_synthesizers
-    synthesizers = get_synthesizers(synthesizers + custom_synthesizers)
-
     # Get list of dataset paths
     aws_access_key_id = os.getenv('AWS_ACCESS_KEY_ID')
     aws_secret_access_key_key = os.getenv('AWS_SECRET_ACCESS_KEY')
@@ -427,7 +471,7 @@ def _compute_scores(
         sdmetrics_metadata = metadata
 
     if len(metrics) > 0:
-        metrics, metric_kwargs = get_metrics(metrics, modality=modality.replace('_', '-'))
+        metrics, metric_kwargs = get_metrics(metrics, modality=modality)
         scores = []
         output['scores'] = scores
         for metric_name, metric in metrics.items():
@@ -1130,8 +1174,8 @@ def _write_metainfo_file(synthesizers, job_args_list, modality, result_writer=No
     }
 
     for synthesizer in synthesizers:
-        if synthesizer not in SDV_SYNTHESIZERS:
-            ext_lib = EXTERNAL_SYNTHESIZER_TO_LIBRARY.get(synthesizer)
+        if synthesizer['name'] not in SDV_SYNTHESIZERS:
+            ext_lib = EXTERNAL_SYNTHESIZER_TO_LIBRARY.get(synthesizer['name'])
             if ext_lib:
                 library_version = version(ext_lib)
                 metadata[f'{ext_lib}_version'] = library_version
@@ -1150,20 +1194,17 @@ def _update_metainfo_file(run_file, result_writer=None):
         result_writer.write_yaml(update, run_file, append=True)
 
 
-def _ensure_uniform_included(synthesizers):
-    if UniformSynthesizer not in synthesizers and UniformSynthesizer.__name__ not in synthesizers:
-        LOGGER.info('Adding UniformSynthesizer to list of synthesizers.')
-        synthesizers.append('UniformSynthesizer')
-
+def _ensure_uniform_included(synthesizers, modality):
+    uniform_class = UniformSynthesizer
+    if modality == 'multi_table':
+        uniform_class = MultiTableUniformSynthesizer
 
-def _ensure_multi_table_uniform_is_included(synthesizers):
     uniform_not_included = bool(
-        MultiTableUniformSynthesizer not in synthesizers
-        and MultiTableUniformSynthesizer.__name__ not in synthesizers
+        uniform_class not in synthesizers and uniform_class.__name__ not in synthesizers
     )
     if uniform_not_included:
-        LOGGER.info('Adding MultiTableUniformSynthesizer to the list of synthesizers.')
-        synthesizers.append('MultiTableUniformSynthesizer')
+        LOGGER.info(f'Adding {uniform_class.__name__} to the list of synthesizers.')
+        synthesizers.append(uniform_class.__name__)
 
 
 def _fill_adjusted_scores_with_none(scores):
@@ -1331,7 +1372,7 @@ def benchmark_single_table(
     if not synthesizers:
         synthesizers = []
 
-    _ensure_uniform_included(synthesizers)
+    _ensure_uniform_included(synthesizers, 'single_table')
     result_writer = LocalResultsWriter()
     if run_on_ec2:
         print("This will create an instance for the current AWS user's account.")  # noqa
@@ -1343,21 +1384,25 @@ def benchmark_single_table(
 
         return None
 
-    _validate_inputs(output_filepath, detailed_results_folder, synthesizers, custom_synthesizers)
-    _create_detailed_results_directory(detailed_results_folder)
-    job_args_list = _generate_job_args_list(
-        limit_dataset_size,
-        sdv_datasets,
-        additional_datasets_folder,
-        sdmetrics,
-        detailed_results_folder,
-        timeout,
-        output_destination,
-        compute_quality_score,
-        compute_diagnostic_score,
-        compute_privacy_score,
+    _validate_output_filepath_and_detailed_results_folder(output_filepath, detailed_results_folder)
+    synthesizers = _import_and_validate_synthesizers(
         synthesizers,
         custom_synthesizers,
+        'single_table',
+    )
+    _create_detailed_results_directory(detailed_results_folder)
+    job_args_list = _generate_job_args_list(
+        limit_dataset_size=limit_dataset_size,
+        sdv_datasets=sdv_datasets,
+        additional_datasets_folder=additional_datasets_folder,
+        sdmetrics=sdmetrics,
+        detailed_results_folder=detailed_results_folder,
+        timeout=timeout,
+        output_destination=output_destination,
+        compute_quality_score=compute_quality_score,
+        compute_diagnostic_score=compute_diagnostic_score,
+        compute_privacy_score=compute_privacy_score,
+        synthesizers=synthesizers,
         s3_client=None,
         modality='single_table',
     )
@@ -1650,7 +1695,13 @@ def benchmark_single_table_aws(
     if not synthesizers:
         synthesizers = []
 
-    _ensure_uniform_included(synthesizers)
+    _ensure_uniform_included(synthesizers, 'single_table')
+    synthesizers = _import_and_validate_synthesizers(
+        synthesizers=synthesizers,
+        custom_synthesizers=None,
+        modality='single_table',
+    )
+
     job_args_list = _generate_job_args_list(
         limit_dataset_size=limit_dataset_size,
         sdv_datasets=sdv_datasets,
@@ -1663,7 +1714,6 @@ def benchmark_single_table_aws(
         compute_privacy_score=compute_privacy_score,
         synthesizers=synthesizers,
         detailed_results_folder=None,
-        custom_synthesizers=None,
         s3_client=s3_client,
         modality='single_table',
     )
@@ -1745,20 +1795,19 @@ def benchmark_multi_table(
 
     Returns:
         pandas.DataFrame:
-            A table containing one row per synthesizer + dataset + metric.
+            A table containing one row per synthesizer + dataset.
     """
     _validate_output_destination(output_destination)
     if not synthesizers:
         synthesizers = []
 
-    _ensure_multi_table_uniform_is_included(synthesizers)
+    _ensure_uniform_included(synthesizers, 'multi_table')
     result_writer = LocalResultsWriter()
 
-    _validate_inputs(
-        output_filepath=None,
-        detailed_results_folder=None,
-        synthesizers=synthesizers,
-        custom_synthesizers=custom_synthesizers,
+    synthesizers = _import_and_validate_synthesizers(
+        synthesizers,
+        custom_synthesizers,
+        'multi_table',
     )
     job_args_list = _generate_job_args_list(
         limit_dataset_size=limit_dataset_size,
@@ -1772,7 +1821,6 @@ def benchmark_multi_table(
         compute_diagnostic_score=compute_diagnostic_score,
         compute_privacy_score=None,
         synthesizers=synthesizers,
-        custom_synthesizers=custom_synthesizers,
         s3_client=None,
         modality='multi_table',
     )

sdgym/metrics.py

@@ -80,15 +80,15 @@ def normalize(self, raw_score):
     ],
 }
 DATA_MODALITY_METRICS = {
-    'single-table': [
+    'single_table': [
         'CSTest',
         'KSComplement',
     ],
-    'multi-table': [
+    'multi_table': [
         'CSTest',
         'KSComplement',
     ],
-    'timeseries': [
+    'sequential': [
         'TSFClassifierEfficacy',
         'LSTMClassifierEfficacy',
         'TSFCDetection',
@@ -104,17 +104,17 @@ def get_metrics(metrics, modality):
         metrics (list):
             List of strings or tuples ``(metric, metric_args)`` describing the metrics.
         modality (str):
-            It must be ``'single-table'``, ``'multi-table'`` or ``'timeseries'``.
+            It must be ``'single_table'``, ``'multi_table'`` or ``'sequential'``.
 
     Returns:
         list, kwargs:
             A list of metrics for the given modality, and their corresponding kwargs.
     """
-    if modality == 'multi-table':
+    if modality == 'multi_table':
         metric_classes = sdmetrics.multi_table.MultiTableMetric.get_subclasses()
-    elif modality == 'single-table':
+    elif modality == 'single_table':
         metric_classes = sdmetrics.single_table.SingleTableMetric.get_subclasses()
-    elif modality == 'timeseries':
+    elif modality == 'sequential':
         metric_classes = sdmetrics.timeseries.TimeSeriesMetric.get_subclasses()
 
     if not metrics:

tests/integration/test_benchmark.py

@@ -510,7 +510,7 @@ def test_benchmark_single_table_no_synthesizers_with_parameters():
         .all()
     )
     assert result['Evaluate_Time'] is None
-    assert result['error'] == 'ValueError: Unknown single-table metric: a'
+    assert result['error'] == 'ValueError: Unknown single_table metric: a'
 
 
 def test_benchmark_single_table_custom_synthesizer():

tests/unit/test__dataset_utils.py

@@ -78,7 +78,7 @@ def test__get_dataset_subset_single_table():
     metadata = {'tables': {'table': {'columns': {f'c{i}': {} for i in range(15)}}}}
 
     # Run
-    result_df, result_meta = _get_dataset_subset(df, metadata, modality='regular')
+    result_df, result_meta = _get_dataset_subset(df, metadata, modality='single_table')
 
     # Assert
     assert len(result_df) <= 1000
@@ -162,7 +162,7 @@ def test__read_zipped_data_single(mock_read):
 
     # Run
     with patch('sdgym._dataset_utils.ZipFile', return_value=mock_zip):
-        data_single = _read_zipped_data('fake.zip', modality='single')
+        data_single = _read_zipped_data('fake.zip', modality='single_table')
 
     # Assert
     assert isinstance(data_single, pd.DataFrame)