[Spot] Disallow sky down spot controller when in-progress spot jobs exist
#1667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Zongheng Yang <[email protected]>
…evitamin/sky-experiments into fix-spot-status-for-cluster-name
Co-authored-by: Zongheng Yang <[email protected]>
Co-authored-by: Zongheng Yang <[email protected]>
…evitamin/sky-experiments into fix-spot-status-for-cluster-name
…evitamin/sky-experiments into disallow-termination-spot
|
May need to merge with master :) |
…nto disallow-termination-spot
…nto disallow-termination-spot
concretevitamin
approved these changes
Feb 10, 2023
There was a problem hiding this comment.
Thanks @Michaelvll. LGTM, this should serve as a much stronger guardrail than before.
sky/cli.py
Outdated
| @@ -2316,7 +2331,7 @@ def _down_or_stop_clusters( | |||
| # TODO(zhwu): We can only have one reserved cluster (spot | |||
| # controller). | |||
| assert len(reserved_clusters) == 1, reserved_clusters | |||
| _hint_for_down_spot_controller(reserved_clusters[0]) | |||
| _hint_or_raise_for_down_spot_controller(reserved_clusters[0]) | |||
|
|
|||
| if not no_confirm: | |||
| click.confirm('Proceed?', | |||
There was a problem hiding this comment.
One option, for discussion: we can check
value = click.prompt('Type 'delete' to proceed', type=str)
if value == 'delete':
...
else:
raise click.Abort() from None
There was a problem hiding this comment.
Great point! I changed it to the following. PTAL. : )
There was a problem hiding this comment.
Kind of feel this is a bit too long; was proposing delete since it's used in AWS console for certain VPC resources. Wdyt? Feel free to do a poll too.
There was a problem hiding this comment.
Good point! I used the I understand because that will remind the user to read the messages above, but I am not feeling strongly about this. Switched back to delete.
Co-authored-by: Zongheng Yang <[email protected]>
concretevitamin
approved these changes
Feb 10, 2023
Co-authored-by: Zongheng Yang <[email protected]>
…n/sky-experiments into disallow-termination-spot
sumanthgenz
pushed a commit
to sumanthgenz/skypilot
that referenced
this pull request
Feb 22, 2023
… exist (skypilot-org#1667) * Expose failure reason for spot jobs * Add failure reason for normal failure * Failure reason hint for sky logs sky-spot-controller * require failure reason for all * Fix the conftest * fix controller name * revert SKYPILOT_USER * Show controller processs logs with sky spot logs for better UX * revert usage user ID * do not overwrite failure reason for resource unavailable * format * lint * address comments * fix comment * Update docs/source/examples/spot-jobs.rst Co-authored-by: Zongheng Yang <[email protected]> * improve readability and refactoring * address comments * format * Add comment * address comments * format * Add failover history to the error rasied by _launch * Add comment * Update sky/spot/recovery_strategy.py Co-authored-by: Zongheng Yang <[email protected]> * refactor * Address comment * Update sky/spot/recovery_strategy.py Co-authored-by: Zongheng Yang <[email protected]> * format * format * fix exception name * Disallow termination of spot controller when spot job is running * Fix message * Fix message * address comments * Update sky/cli.py Co-authored-by: Zongheng Yang <[email protected]> * Fix UX * Update sky/cli.py Co-authored-by: Zongheng Yang <[email protected]> * change confirm str to `delete` --------- Co-authored-by: Zongheng Yang <[email protected]>
sumanthgenz
pushed a commit
to sumanthgenz/skypilot
that referenced
this pull request
Mar 15, 2023
… exist (skypilot-org#1667) * Expose failure reason for spot jobs * Add failure reason for normal failure * Failure reason hint for sky logs sky-spot-controller * require failure reason for all * Fix the conftest * fix controller name * revert SKYPILOT_USER * Show controller processs logs with sky spot logs for better UX * revert usage user ID * do not overwrite failure reason for resource unavailable * format * lint * address comments * fix comment * Update docs/source/examples/spot-jobs.rst Co-authored-by: Zongheng Yang <[email protected]> * improve readability and refactoring * address comments * format * Add comment * address comments * format * Add failover history to the error rasied by _launch * Add comment * Update sky/spot/recovery_strategy.py Co-authored-by: Zongheng Yang <[email protected]> * refactor * Address comment * Update sky/spot/recovery_strategy.py Co-authored-by: Zongheng Yang <[email protected]> * format * format * fix exception name * Disallow termination of spot controller when spot job is running * Fix message * Fix message * address comments * Update sky/cli.py Co-authored-by: Zongheng Yang <[email protected]> * Fix UX * Update sky/cli.py Co-authored-by: Zongheng Yang <[email protected]> * change confirm str to `delete` --------- Co-authored-by: Zongheng Yang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1666
With this PR the following message will show if
sky downthe spot controller with in-progress spot jobs:Tested:
sky down sky-spot-controller-<hash>with in-progress spot jobs