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

[FluidStack][API] Update FluidStack to new API #3799

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

mjibril
Copy link
Contributor

@mjibril mjibril commented Jul 30, 2024

* Update FluidStack to new API

* Simplify deployment by removing CUDA installation and using pre-built images

Co-authored-by: Mubarak Jibril  <[email protected]>
  • Update FluidStack to new API
  • Simplify deployment by removing CUDA installation and using pre-built images
  • Smoke tests

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    sky launch -c test-single-instance --cloud fluidstack echo hi
    sky check
  • All smoke tests: pytest tests/test_smoke.py
    pytest tests/test_smoke.py --fluidstack --terminate-on-failure
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

mjibril added 2 commits July 30, 2024 14:21
    * Update FluidStack to new API

    * Simplify deployment by removing CUDA installation and using pre-built images

    Co-authored-by: Mubarak Jibril  <[email protected]>
Copy link
Collaborator

@Michaelvll Michaelvll 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 migrating the fluidstack API @mjibril! It mostly looks good to me. Please see the comments below, mostly for tests.

As we mentioned offline, we saw an issue terminating existing instances., for which we should probably fix.

sky/clouds/fluidstack.py Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
@mjibril mjibril force-pushed the fluidstack-new-api branch 4 times, most recently from e341c22 to f02dc2f Compare August 5, 2024 19:36
Comment on lines +3821 to +3824
pytest.mark.no_fluidstack


Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pytest.mark.no_fluidstack
@pytest.mark.no_fluidstack

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, we may want to add the same decorator for the test_skyserve_fast_update below on L3861 as well.

@mjibril mjibril force-pushed the fluidstack-new-api branch from f02dc2f to bd73b7d Compare August 6, 2024 11:34
Copy link
Collaborator

@Michaelvll Michaelvll 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 the fix @mjibril! Just a reminder for the comment here

Otherwise, it looks good to me!

@mjibril mjibril force-pushed the fluidstack-new-api branch from bd73b7d to fd25ccd Compare August 21, 2024 13:09
@mjibril mjibril force-pushed the fluidstack-new-api branch from fd25ccd to 8b40139 Compare August 21, 2024 13:19
@Michaelvll Michaelvll added this pull request to the merge queue Aug 21, 2024
@Michaelvll
Copy link
Collaborator

Thanks for the update @mjibril and the excellent work! Just merged this PR to master.

Merged via the queue into skypilot-org:master with commit 6101a04 Aug 21, 2024
20 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