Skip to content

Make BenchmarkConfig.compute support instance configuration in YAML#592

Open
R-Palazzo wants to merge 8 commits intomainfrom
issue-586-compute-yaml
Open

Make BenchmarkConfig.compute support instance configuration in YAML#592
R-Palazzo wants to merge 8 commits intomainfrom
issue-586-compute-yaml

Conversation

@R-Palazzo
Copy link
Copy Markdown
Collaborator

Resolve #586
CU-86b94u5fd

@R-Palazzo R-Palazzo requested review from amontanez24 and fealho April 10, 2026 15:43
@R-Palazzo R-Palazzo self-assigned this Apr 10, 2026
@R-Palazzo R-Palazzo requested a review from a team as a code owner April 10, 2026 15:43
@sdv-team
Copy link
Copy Markdown
Contributor

@R-Palazzo R-Palazzo removed the request for review from a team April 10, 2026 15:44
gpu_type: nvidia-tesla-t4
gpu_count: 1
swap_gb: 64
name_prefix: sdgym-run
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I went with this approach to stay consistent with what already exists.

I wonder whether in the future it would be better to support a higher-level image config that resolves the image for the user, keep boot_image only as an explicit override, or support both.

image:
    flavor: dlvm
    os: ubuntu
    os_version: "22.04"
    cuda: "12.8"

@R-Palazzo R-Palazzo requested a review from fealho April 10, 2026 16:37
Copy link
Copy Markdown
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

Just one comment!


def resolve_compute(compute):
"""Resolve the compute configuration based on the benchmark config."""
return _resolve_compute_gcp(compute)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we add a check here that determines the service we are using for compute and then do

if service == 'gcp':
    return _resolve_compute_gcp()
elif service == 'aws:
    ...
else:
    raise Error('service must be one of ...')

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, done in ad42d49

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.12%. Comparing base (fb45e14) to head (ad42d49).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
- Coverage   85.15%   85.12%   -0.03%     
==========================================
  Files          41       40       -1     
  Lines        3563     3544      -19     
==========================================
- Hits         3034     3017      -17     
+ Misses        529      527       -2     
Flag Coverage Δ
integration 45.11% <0.00%> (+0.24%) ⬆️
unit 80.78% <100.00%> (-0.05%) ⬇️

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.

@R-Palazzo R-Palazzo force-pushed the issue-586-compute-yaml branch from 4aa044b to ad42d49 Compare April 13, 2026 09:00
@R-Palazzo R-Palazzo requested a review from amontanez24 April 13, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make BenchmarkConfig.compute support instance configuration in YAML

4 participants