Move the current benchmark configs to yaml files by R-Palazzo · Pull Request #566 · sdv-dev/SDGym

R-Palazzo · 2026-03-05T16:18:17Z

Resolve #545
CU-86b8h52bh

It's a pretty big PR, thanks in advance for your review :)

Currently the BenchmarkLauncher only works for GCP.
To make it work on other compute servie (e.g. AWS) we will have to define benchmark methods that have similar parameter as the gcp ones (with credentials/compute_config):

SDGym/sdgym/_benchmark/benchmark.py

Line 350 in 3c2a248

def _benchmark_compute_gcp(

sdv-team · 2026-03-05T16:18:23Z

Task linked: CU-86b8h52bh SDGym - Move the current benchmark configs to yaml files #545

R-Palazzo · 2026-03-05T18:12:53Z

sdgym/_benchmark_launcher/benchmark_base.yaml

+  compute_privacy_score: false
+
+compute:
+  service: 'gcp'


In this PR, we only indicate which compute service to use. In a future issue, we will move the compute config defined here

SDGym/sdgym/_benchmark/config_utils.py

Line 19 in 3c2a248

'gcp': {

Inside this yaml file also

sdgym/_benchmark_launcher/benchmark_launcher.py

codecov · 2026-03-05T20:21:37Z

Codecov Report

❌ Patch coverage is 93.37748% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.30%. Comparing base (0d8b7f8) to head (7f06606).
⚠️ Report is 1 commits behind head on feature/benchmark_launcher.

Files with missing lines	Patch %	Lines
sdgym/_benchmark_launcher/_validation.py	90.90%	11 Missing ⚠️
sdgym/_benchmark_launcher/benchmark_launcher.py	78.78%	7 Missing ⚠️
sdgym/_benchmark_launcher/benchmark_config.py	98.21%	1 Missing ⚠️
sdgym/_benchmark_launcher/utils.py	98.63%	1 Missing ⚠️

Additional details and impacted files

@@                      Coverage Diff                       @@
##           feature/benchmark_launcher     #566      +/-   ##
==============================================================
+ Coverage                       82.41%   83.30%   +0.88%     
==============================================================
  Files                              33       38       +5     
  Lines                            2923     3198     +275     
==============================================================
+ Hits                             2409     2664     +255     
- Misses                            514      534      +20

Flag	Coverage Δ
integration	`49.37% <0.33%> (-4.62%)`	⬇️
unit	`78.42% <93.37%> (+1.34%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pvk-developer

I think this is looking very good. I would suggest going over the _validation.py and try to modularize it a little bit if you have the time since it is hard to navigate the long functions in there.

sdgym/_benchmark_launcher/benchmark_launcher.py

sdgym/_benchmark_launcher/__init__.py

sdgym/_benchmark_launcher/benchmark_config.py

sdgym/_benchmark_launcher/benchmark_multi_table.yaml

R-Palazzo · 2026-03-11T13:56:12Z

sdgym/_benchmark_launcher/benchmark_base.yaml

@@ -0,0 +1,21 @@
+method_params:
+  timeout: 345600
+  output_destination: 's3://sdgym-benchmark/Debug/Benchmark_Launcher/'


TODO: Update before merging

R-Palazzo · 2026-03-11T14:39:14Z

.github/workflows/run_benchmark.yml

          )
          "
-          python -m pip install "sdgym[all]"
+          python -m pip install "sdgym[all] @ git+https://github.com/sdv-dev/SDGym.git@issue-532-define-yaml-files"


TODO: Revert before merging

amontanez24

I think we should restructure the credentials a bit. Lets define a format for the dict like:

gcp:
    GCP_SERVICE_ACCOUNT_JSON: ...
    GCP_SERVICE_ACCOUNT_PATH: ...
    GCP_PROJECT_ID: ...
    GCP_ZONE: ...
sdv_enterprise:
    SDV_ENTERPRISE_USERNAME: ...
    SDV_ENTERPRISE_LICENSE_KEY: ...
aws:
    AWS_ACCESS_KEY_ID: ...
    AWS_SECRET_ACCESS_KEY: ...

We then load this dict and check for expected keys. If they key isn't present we try to load from the environment. So if someone is running with GCP, we would attempt to load all the expected GCP keys from the dict and then check the environment if the dict doesn't have it.

If you want to make the names less redundant, you can remove the prefixes ('SDV', 'AWS', 'GCP') and say that if you are using environment variables you should store it as {service_name}_{key_name}.

sdgym/_benchmark_launcher/benchmark_base.yaml

amontanez24 · 2026-03-11T19:27:58Z

sdgym/_benchmark_launcher/_validation.py

+def _get_credentials(credential_locations):
+    """Get resolved credentials dict."""
+    config = credential_locations or {}
+    filepath = config.get('credential_filepath')


in the case where the filepath is provided, is the structure of the credentials dict the same as credential_locations?

sdgym/_benchmark_launcher/benchmark_config.py

amontanez24 · 2026-03-11T19:35:30Z

sdgym/_benchmark_launcher/_validation.py

+    sig = inspect.signature(method_to_run)
+    required = {
+        parameter.name
+        for parameter in sig.parameters.values()
+        if parameter.default is inspect.Parameter.empty
+        and parameter.kind
+        in (inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY)
+    }
+    required_from_yaml = required - _INJECTED_PARAMS
+    missing = required_from_yaml - set(method_params)
+    if missing:
+        errors.append(
+            f'method_params: missing required parameters for {method_to_run.__name__}:'
+            f' {sorted(missing)}'
+        )


is this necessary? Can't we just have defaults for whatever they don't provide? The only one that I can see as required is the output destination, and I don't think that one should be grouped with the other parameters.

I removed it in 133a551.

amontanez24 · 2026-03-11T19:36:40Z

sdgym/_benchmark_launcher/benchmark_config.py

+        'method_params': dict of parameters to pass to the benchmark method (e.g. timeout),
+        'credentials': dict specifying how to resolve credentials (e.g. from env vars or a file),
+        'compute': dict specifying the compute configuration (e.g. service: 'gcp'),
+        'instance_jobs': list of dicts, each specifying a combination of synthesizers and datasets:


I think the output destination should be specified with the jobs

I was thinking it makes more sense to have all the results for a benchmark in the same location, but you’re right we could also set one output_destination per instance_job. Both options work for me. Let me know which we prefer.

I think at some point Kalyan told me we may want to have different output destinations so we should put it with the jobs

sdgym/_benchmark_launcher/_validation.py

amontanez24

I think we're almost there! I left a couple comments and think we should move the output destination to the jobs but besides that looks good

sdgym/_benchmark_launcher/_validation.py

sdgym/_benchmark_launcher/utils.py

amontanez24 · 2026-03-13T18:00:50Z

sdgym/_benchmark_launcher/benchmark_config.py

+        'method_params': dict of parameters to pass to the benchmark method (e.g. timeout),
+        'credentials': dict specifying how to resolve credentials (e.g. from env vars or a file),
+        'compute': dict specifying the compute configuration (e.g. service: 'gcp'),
+        'instance_jobs': list of dicts, each specifying a combination of synthesizers and datasets:


I think at some point Kalyan told me we may want to have different output destinations so we should put it with the jobs

R-Palazzo self-assigned this Mar 5, 2026

R-Palazzo changed the title ~~Issue 532 define yaml files~~ Move the current benchmark configs to yaml files Mar 5, 2026

R-Palazzo requested review from amontanez24 and pvk-developer March 5, 2026 18:08

R-Palazzo marked this pull request as ready for review March 5, 2026 18:09

R-Palazzo requested a review from a team as a code owner March 5, 2026 18:09

R-Palazzo commented Mar 5, 2026

View reviewed changes

sdgym/_benchmark_launcher/benchmark_launcher.py Show resolved Hide resolved

pvk-developer reviewed Mar 6, 2026

View reviewed changes

R-Palazzo added 9 commits March 10, 2026 10:03

progress 545

706fc74

progress

af06677

progress 2

4fa0081

progress 3

952dd7b

progress 4

19404a7

progress 5

50af9ee

progress 6

39623ad

progress 7

c4edfa5

cleaning

f670ba5

R-Palazzo force-pushed the issue-532-define-yaml-files branch from abe9001 to f670ba5 Compare March 10, 2026 10:03

R-Palazzo added 3 commits March 10, 2026 11:23

address comments

1a5ef6f

fix tests

df742dd

update

612c429

R-Palazzo requested a review from pvk-developer March 11, 2026 12:41

R-Palazzo added 2 commits March 11, 2026 12:52

fix test

e6b57fc

update pyproject

5c42adb

R-Palazzo commented Mar 11, 2026

View reviewed changes

amontanez24 reviewed Mar 11, 2026

View reviewed changes

R-Palazzo added 2 commits March 12, 2026 17:49

fix credential handling

629b80c

remove method parameter check

133a551

R-Palazzo requested a review from amontanez24 March 12, 2026 18:24

R-Palazzo added 2 commits March 13, 2026 11:25

test end to end

28bffd6

fix

9ae9629

pvk-developer approved these changes Mar 13, 2026

View reviewed changes

amontanez24 reviewed Mar 13, 2026

View reviewed changes

R-Palazzo added 2 commits March 16, 2026 14:51

move output_destination to instance_jobs

36957fd

address comments

0b07c0c

R-Palazzo requested a review from amontanez24 March 16, 2026 15:19

fix yaml files

7f06606

amontanez24 approved these changes Mar 17, 2026

View reviewed changes

R-Palazzo merged commit 6bdfdae into feature/benchmark_launcher Mar 17, 2026
51 checks passed

R-Palazzo deleted the issue-532-define-yaml-files branch March 17, 2026 12:16

R-Palazzo added a commit that referenced this pull request Mar 19, 2026

Move the current benchmark configs to yaml files (#566)

dd99c35

R-Palazzo added a commit that referenced this pull request Mar 24, 2026

Move the current benchmark configs to yaml files (#566)

b33605c

Conversation

R-Palazzo commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdv-team commented Mar 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pvk-developer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amontanez24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amontanez24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

R-Palazzo commented Mar 5, 2026 •

edited

Loading

codecov bot commented Mar 5, 2026 •

edited

Loading