Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Commit: Add vultr support #110

Merged
merged 7 commits into from
Jan 8, 2025
Merged

Initial Commit: Add vultr support #110

merged 7 commits into from
Jan 8, 2025

Conversation

Bihan
Copy link
Contributor

@Bihan Bihan commented Dec 19, 2024

No description provided.

@Bihan Bihan marked this pull request as draft December 19, 2024 17:05
@Bihan Bihan marked this pull request as ready for review December 19, 2024 17:08
Copy link
Collaborator

@jvstme jvstme left a comment

Choose a reason for hiding this comment

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

@Bihan, thanks for the PR. I looked through it briefly and left some comments, please feel free to respond to them if you have the time. I may add a few more comments later though

src/gpuhunt/_internal/catalog.py Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/tests/providers/test_vultr.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jvstme jvstme left a comment

Choose a reason for hiding this comment

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

@Bihan, thanks for the update. I looked through the changes and the resulting catalog (python -m gpuhunt vultr --output vultr.csv) and found a few more edge cases that seem to yield incorrect results. Hope you can address these too

src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/_internal/catalog.py Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/tests/providers/test_vultr.py Show resolved Hide resolved
@Bihan Bihan requested a review from jvstme December 27, 2024 09:05
Copy link
Collaborator

@jvstme jvstme left a comment

Choose a reason for hiding this comment

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

@Bihan, thanks for the update, there are just a few comments left before this PR is good to merge.

src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
@Bihan
Copy link
Contributor Author

Bihan commented Jan 6, 2025

@jvstme I have modified get_bare_metal_plans function as below:

def get_bare_metal_plans(plan: dict, location: str) -> Optional[RawCatalogItem]:
    gpu_details = BARE_METAL_GPU_DETAILS.get(plan["id"], None)
    return RawCatalogItem(
        instance_name=plan["id"],
        location=location,
        price=plan["hourly_cost"],
        cpu=plan["cpu_threads"],
        memory=plan["ram"] / 1024,
        gpu_count=gpu_details[0] if gpu_details else 0,
        gpu_name=gpu_details[1] if gpu_details else None,
        gpu_memory=gpu_details[2] if gpu_details else None,
        gpu_vendor=get_gpu_vendor(gpu_details[1]) if gpu_details else None,
        spot=False,
        disk_size=plan["disk"],
    )

I think this solves the above issues.

Regarding L40s, it is still not available to verify it experimentally. Currently I have considered vbm-64c-2048gb-8-l40-gpu as L40s.

Copy link
Collaborator

@jvstme jvstme left a comment

Choose a reason for hiding this comment

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

@Bihan, thanks, I added a couple more suggestions - please see if you agree with them and we can merge the PR then.

src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
src/gpuhunt/providers/vultr.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jvstme jvstme left a comment

Choose a reason for hiding this comment

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

👍👍

@jvstme jvstme merged commit 91eca44 into dstackai:main Jan 8, 2025
6 checks passed
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.

2 participants