Add support for conditional exclusion of features (specific to a cloud) during provisioning #2293
Conversation
sky/backends/cloud_vm_ray_backend.py
Outdated
|
|
||
| if self._requested_features != granted_features: | ||
| logger.info( | ||
| f'{colorama.Fore.CYAN}The following features will be skipped since they are not supported by {to_provision.cloud.__class__.__name__} and were deemed optional : ' |
There was a problem hiding this comment.
| f'{colorama.Fore.CYAN}The following features will be skipped since they are not supported by {to_provision.cloud.__class__.__name__} and were deemed optional : ' | |
| f'{colorama.Fore.CYAN}The following features will be skipped since they are not supported by {to_provision.cloud._REPR} and were deemed optional : ' |
There was a problem hiding this comment.
Checked and print(to_provision.cloud) gives Kubernetes so just {to_provision.cloud} should be fine
There was a problem hiding this comment.
IIUC using cloud._REPR is some code convention. cc @concretevitamin for a look
There was a problem hiding this comment.
Yeah, it's supposed to be used either via {to_provision.cloud} or {repr(to_provision.cloud)} to not access private attrs directly.
There was a problem hiding this comment.
Changed to {to_provision.cloud}
sky/clouds/cloud.py
Outdated
| cls, requested_features: Set[CloudImplementationFeatures], | ||
| config: ExcludableFeatureCheckConfig | ||
| ) -> Set[CloudImplementationFeatures]: | ||
| """Returns the features that can be granted by the cloud implementation. |
There was a problem hiding this comment.
Would be great if adding a comment saying the cloud.check_features_are_supported checking is moved to here 🙂
There was a problem hiding this comment.
Shall we also refactor the implementation in here?
Line 303 in f5526d4
There was a problem hiding this comment.
Sounds good, I can work on it
There was a problem hiding this comment.
Checked on this, looks like cluster_name is not available in this method.
| lambda config: config.cluster_name.startswith( | ||
| 'sky-spot-controller-') |
There was a problem hiding this comment.
| lambda config: config.cluster_name.startswith( | |
| 'sky-spot-controller-') | |
| lambda config: config.cluster_name == spot.SPOT_CONTROLLER_NAME |
There was a problem hiding this comment.
I tried this but it was giving a circular import error
There was a problem hiding this comment.
Oh ic... Maybe you could copy-paste the f'sky-spot-controller-{common_utils.get_user_hash()}' to here? Or if @concretevitamin has any other ideas?
There was a problem hiding this comment.
Yeah happy to explicitly check for f'sky-spot-controller-{common_utils.get_user_hash()}', I'll go with whatever is the preference.
There was a problem hiding this comment.
Happy with either way.
I just realized this PR is merging into the k8s branch. cc @romilbhardwaj to take a look too. On the surface it seems like we should make k8s.py importing spot work (the latter should not import k8s?).
hemildesai
force-pushed
the
hd/k8s-spot-no-autostop
branch
from
5bc3e57 to
c6085dd
Compare
Co-authored-by: Tian Xia <cblmemo@gmail.com>
hemildesai
force-pushed
the
hd/k8s-spot-no-autostop
branch
from
c6085dd to
7daed9d
Compare
|
Hey @hemildesai - this PR got closed when we merged #2096 into master. For our next sprint, we'll be switching to |
|
Couldn't reopen so created #2348 |
This PR adds a backbone to conditionally exclude some unsupported features in a cloud instead of raising an error if even one of the requested features is unsupported. It accomplishes this by maintaining a per cloud map of excludable features -> exclusion condition. During provisioning, if the exclusion condition is met for any of the requested features, it excludes that feature from the corresponding
check_features_are_supported. As of now, this aims to address #2249This is a proof of concept PR and I've tested in locally (only have k8s cloud enabled). If this is a direction that we want to proceed in, I can work on adding tests for it and running existing smoke/integration tests.
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_namebash tests/backward_comaptibility_tests.sh