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

[DigitalOcean] droplet integration #3832

Open
wants to merge 79 commits into
base: master
Choose a base branch
from

Conversation

asaiacai
Copy link
Contributor

@asaiacai asaiacai commented Aug 14, 2024

This adds digital ocean droplets to the sky.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • All smoke tests: pytest tests/test_smoke.py --do

@moinnadeem
Copy link

@asaiacai I would be really interested in using this work -- do you know when you might be able to land this PR?

@asaiacai
Copy link
Contributor Author

asaiacai commented Aug 31, 2024 via email

@asaiacai asaiacai marked this pull request as ready for review September 11, 2024 00:07
@asaiacai
Copy link
Contributor Author

@Michaelvll i think this PR is finally ready, once you have a free moment! I've currently disabled tests for gpu droplets as they are still in early access, but I think I got the remaining tests to work on normal CPU droplets fine. Let me know if you find any issues.

@asaiacai
Copy link
Contributor Author

also this catalog update is required to pass the sky serve requests since I added cheaper 2vcpu instances.

skypilot-org/skypilot-catalog#83

Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks for this amazing work @asaiacai ! This would be very helpful. Left some discussions ;)

docs/source/getting-started/installation.rst Outdated Show resolved Hide resolved
sky/adaptors/do.py Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/templates/do-ray.yml.j2 Show resolved Hide resolved
tests/test_smoke.py Show resolved Hide resolved
tests/skyserve/update/bump_version_after.yaml Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Show resolved Hide resolved
asaiacai and others added 2 commits September 17, 2024 12:45
Co-authored-by: Tian Xia <cblmemo@gmail.com>
Co-authored-by: Tian Xia <cblmemo@gmail.com>
@asaiacai asaiacai requested a review from cblmemo October 27, 2024 23:35
@asaiacai
Copy link
Contributor Author

@cblmemo reran smoke, but if you have a moment to review again, thanks in advance!

@asaiacai
Copy link
Contributor Author

asaiacai commented Nov 6, 2024

@cblmemo digital ocean released a new image for AI/ML with docker. It's a lot more bloated than the ubuntu-22.04 images that were previously used for the CPU VMs but it does ship with docker, and i've removed the docker wait. please let me know if you need anything else for this PR.

Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks @asaiacai for this amazing contribution! Looks mostly good to me. After those nits get resolved and smoke test passed, it should be ready to go!

sky/clouds/do.py Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/do.py Outdated Show resolved Hide resolved
sky/clouds/service_catalog/do_catalog.py Show resolved Hide resolved
sky/templates/do-ray.yml.j2 Outdated Show resolved Hide resolved
Comment on lines +1591 to +1592
@pytest.mark.do
def test_do_job_queue_with_docker(generic_cloud: str, image_id: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to reuse the previous function and add another argument for accelerator to use? same for other tests in this file. You can checkout @pytest.mark.parametrize

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.

3 participants