Query cloud specific env vars in task setup #2347
Merged
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
6 tasks
There was a problem hiding this comment.
This is super timely for #2328, which needs access to envvars set by GKE to setup GPU access. Thanks for the great work @hemildesai!
Tested it, works nicely with sky launch. Left some comments about some critical edge cases.
sky/backends/cloud_vm_ray_backend.py
Outdated
| @@ -2828,6 +2828,11 @@ def _setup(self, handle: CloudVmRayResourceHandle, task: task_lib.Task, | |||
| style = colorama.Style | |||
| fore = colorama.Fore | |||
|
|
|||
| if isinstance(handle.launched_resources.cloud, clouds.Kubernetes): | |||
There was a problem hiding this comment.
_setup() is not invoked when a user runs sky exec. We may need to move/copy this block elsewhere to make sure k8s envvars are set for the run part of the YAML too if the users runs sky exec.
There was a problem hiding this comment.
I can create a utility to update env vars and call it in both _setup and _execute
sky/backends/cloud_vm_ray_backend.py
Outdated
| if isinstance(handle.launched_resources.cloud, clouds.Kubernetes): | ||
| cloud_env_vars = handle.launched_resources.cloud.query_env_vars( | ||
| handle.cluster_name) | ||
| task.update_envs(cloud_env_vars) |
There was a problem hiding this comment.
If a user has defined an envvar that conflicts with envvar from Kubernetes, this will override users' envvar and replace it with the value from Kubernetes. Example:
# In a GPU cluster on GKE, nvidia specific envvars are set dynamically by k8s, including LIBRARY_PATH=/usr/local/cuda/lib64/stubs
# Here the user wants to set their own cuda path in LIBRARY_PATH, but it doesn't work
run : |
echo run
echo $LIBRARY_PATH
nvidia-smi
envs:
LIBRARY_PATH: /my/cuda/path
# Output:
# run
# /usr/local/cuda/lib64/stubs
# ...
We may want to do something here to make sure envvars set by users take precedence. E.g., we could copy current env vars to temp -> update with k8s envs -> update with current env vars. Also open to other suggestions.
There was a problem hiding this comment.
That sounds good, I will update to give precedence to user defined env vars.
sky/clouds/kubernetes.py
Outdated
Comment on lines
428
to
431
| line.split('=', 1) | ||
| for line in response.split('\n') | ||
| if '=' in line and | ||
| common_utils.is_valid_env_var(line.split('=', 1)[0]) |
There was a problem hiding this comment.
Nit: feel free to break this comprehension into multiple lines if it helps improve readability :)
sky/task.py
Outdated
| @@ -18,6 +18,7 @@ | |||
| from sky.skylet import constants | |||
| from sky.utils import schemas | |||
| from sky.utils import ux_utils | |||
| from sky.utils.common_utils import is_valid_env_var | |||
There was a problem hiding this comment.
we typically follow google style guide here:
from sky.utils import common_utils
...
common_utils. is_valid_env_var(...)
| namespace, | ||
| label_selector=f'skypilot-cluster={name},ray-node-type=head' | ||
| ).items[0] | ||
| response = kubernetes.stream()( |
There was a problem hiding this comment.
Is it possible to set a timeout on this stream call (like we have _request_timeout=kubernetes.API_TIMEOUT in the rest of our code)?
There was a problem hiding this comment.
Yes it should be. I'll add the timeout.
|
Addressed all comments in b95ed1e. Open to suggestions for better refactoring of |
|
Thanks @hemildesai! In addition to envs in YAML, I also tested inline envs with |
romilbhardwaj
added a commit
that referenced
this pull request
Aug 25, 2023
* Working Ray K8s node provider based on SSH * wip * working provisioning with SkyPilot and ssh config * working provisioning with SkyPilot and ssh config * Updates to master * ray2.3 * Clean up docs * multiarch build * hacking around ray start * more port fixes * fix up default instance selection * fix resource selection * Add provisioning timeout by checking if pods are ready * Working mounting * Remove catalog * fixes * fixes * Fix ssh-key auth to create unique secrets * Fix for ContainerCreating timeout * Fix head node ssh port caching * mypy * lint * fix ports * typo * cleanup * cleanup * wip * Update setup * readme updates * lint * Fix failover * Fix failover * optimize setup * Fix sync down logs for k8s * test wip * instance name parsing wip * Fix instance name parsing * Merge fixes for query_status * [k8s_cloud] Delete k8s service resources. (#2105) Delete k8s service resources. - 'sky down' for Kubernetes cloud to remove cluster service resources. * Status refresh WIP * refactor to kubernetes adaptor * tests wip * clean up auth * wip tests * cli * cli * sky local up/down cli * cli * lint * lint * lint * Speed up kind cluster creation * tests * lint * tests * handling for non-reachable clusters * Invalid kubeconfig handling * Timeout for sky check * code cleanup * lint * Do not raise error if GPUs requested, return empty list * Address comments * comments * lint * Remove public key upload * GPU support init * wip * add shebang * comments * change permissions * remove chmod * merge 2241 * add todo * Handle kube config management for sky local commands (#2253) * Set current-context (if availablee) after sky local down and remove incorrect prompt in sky local up * Warn user of kubeconfig context switch during sky local up * Use Optional instead of Union * Switch context in create_cluster if cluster already exists. * fix typo * update sky check error msg after sky local down * lint * update timeout check * fix import error * Fix kube API access from within cluster (load_incluster_auth) * lint * lint * working autodown and sky status -r * lint * add test_kubernetes_autodown * lint * address comments * address comments * lint * deletion timeouts wip * [k8s_cloud] Ray pod not created under current context namespace. (#2302) 'namespace' exists under 'context' key. * head ssh port namespace fix * [k8s-cloud] Typo in sky local --help. (#2308) Typo. * [k8s-cloud] Set build_image.sh to be executable. (#2307) * Set build_image.sh to be executable. * Use TAG to easily switch between registries. * remove ingress * remove debug statements * UX and readme updates * lint * fix logging for 409 retry * lint * lint * Debug dockerfile * wip * Fix GPU image * Query cloud specific env vars in task setup (#2347) * Query cloud specific env vars in task setup * Make query_env_vars specific to Kubernetes cloud * Address PR comments * working GPU type selection for GKE and EKS. GFD needs work. * TODO for auto-detection * Add image toggling for CPU/GPU * Add image toggling for CPU/GPU * Fix none acce_type * remove memory from j2 * Make resnet examples run again * lint * v100 readme * dockerfile and smoketest * fractional cpu and mem * nits * refactor utils * lint and cleanup * lint and cleanup * lint and cleanup * lint and cleanup * lint and cleanup * lint and cleanup * lint * lint * manual lint * manual isort * test readme update * Remove EKS * lint * add gpu labeler * updates * lint * update script * ux * fix formatter * test update * test update * fix test_optimizer_dryruns * docs * cleanup * test readme update * lint * lint * Update imagepullpolicy to always * update image build * typing hints * update docstr * some comments * refactor * refactor * lint * lint * update gke cmd * update monkeypatch * yapf * comments * increase default mem when GPU task * increase default mem when GPU task * fix test_optimize_speed * lint * Add CPU+Mem based early filtering and better debug logging * lint * fix test_optimizer * fixes * fix k8s port fetch logic * clean up instance make logic * increase default mem when GPU task * lint * clean up fit check logic * add catalog todo * eksctl update * update readme and gpu_labeler with comments * monkeypatch in enable_all_clouds * change to T4 --------- Co-authored-by: Avi Weit <[email protected]> Co-authored-by: Hemil Desai <[email protected]>
romilbhardwaj
added a commit
that referenced
this pull request
Sep 16, 2023
* Working Ray K8s node provider based on SSH * wip * working provisioning with SkyPilot and ssh config * working provisioning with SkyPilot and ssh config * Updates to master * ray2.3 * Clean up docs * multiarch build * hacking around ray start * more port fixes * fix up default instance selection * fix resource selection * Add provisioning timeout by checking if pods are ready * Working mounting * Remove catalog * fixes * fixes * Fix ssh-key auth to create unique secrets * Fix for ContainerCreating timeout * Fix head node ssh port caching * mypy * lint * fix ports * typo * cleanup * cleanup * wip * Update setup * readme updates * lint * Fix failover * Fix failover * optimize setup * Fix sync down logs for k8s * test wip * instance name parsing wip * Fix instance name parsing * Merge fixes for query_status * [k8s_cloud] Delete k8s service resources. (#2105) Delete k8s service resources. - 'sky down' for Kubernetes cloud to remove cluster service resources. * Status refresh WIP * refactor to kubernetes adaptor * tests wip * clean up auth * wip tests * cli * cli * sky local up/down cli * cli * lint * lint * lint * Speed up kind cluster creation * tests * lint * tests * handling for non-reachable clusters * Invalid kubeconfig handling * Timeout for sky check * code cleanup * lint * Do not raise error if GPUs requested, return empty list * Address comments * comments * lint * Remove public key upload * GPU support init * wip * add shebang * comments * change permissions * remove chmod * merge 2241 * add todo * Handle kube config management for sky local commands (#2253) * Set current-context (if availablee) after sky local down and remove incorrect prompt in sky local up * Warn user of kubeconfig context switch during sky local up * Use Optional instead of Union * Switch context in create_cluster if cluster already exists. * fix typo * update sky check error msg after sky local down * lint * update timeout check * fix import error * Fix kube API access from within cluster (load_incluster_auth) * lint * lint * working autodown and sky status -r * lint * add test_kubernetes_autodown * lint * address comments * address comments * lint * deletion timeouts wip * [k8s_cloud] Ray pod not created under current context namespace. (#2302) 'namespace' exists under 'context' key. * head ssh port namespace fix * [k8s-cloud] Typo in sky local --help. (#2308) Typo. * [k8s-cloud] Set build_image.sh to be executable. (#2307) * Set build_image.sh to be executable. * Use TAG to easily switch between registries. * remove ingress * remove debug statements * UX and readme updates * lint * fix logging for 409 retry * lint * lint * Debug dockerfile * wip * Fix GPU image * Query cloud specific env vars in task setup (#2347) * Query cloud specific env vars in task setup * Make query_env_vars specific to Kubernetes cloud * Address PR comments * working GPU type selection for GKE and EKS. GFD needs work. * TODO for auto-detection * Add image toggling for CPU/GPU * Add image toggling for CPU/GPU * Fix none acce_type * remove memory from j2 * Make resnet examples run again * lint * v100 readme * dockerfile and smoketest * fractional cpu and mem * nits * refactor utils * lint and cleanup * lint and cleanup * lint and cleanup * lint and cleanup * lint and cleanup * lint and cleanup * lint * lint * manual lint * manual isort * test readme update * Remove EKS * lint * add gpu labeler * updates * lint * update script * ux * fix formatter * test update * test update * fix test_optimizer_dryruns * docs * cleanup * test readme update * lint * lint * [k8s_cloud_beta1] Add sshjump host support. (#2369) * Update build image * fix image path * fix merge * cleanup * lint * fix utils ref * typo * refactor pod creation * lint * merge fixes * portfix * merge fixes * [k8s_cloud_beta1] Sky down for a cluster deployed in Kubernetes to possibly remove sshjump pod. (#2425) * Sky down for a kubernetes cluster to possibly terminate sshjump pod. - If the related sshjump pod is being reported as its main container not have been started, then remove its pod and service. This is to minimize the chances for remaining with dangling sshjump pod. * Remove sshjump service in case of an failure to analyze sshjump. - remove _request_timeout as it might not be needed due to terminationGracePeriodSeconds being set in sshjump template. * Move sshjump analysis to kubernetes_utils. * Apply changes per ./format.sh. * Minor comment rephrase. * Use sshjump_name from ray pod label. - rather than from clouds.Kubernetes * cleanup * Add networking benchmarks * comment * comment * lint * autodown fixes * lint * fix label * [k8s_cloud_beta1] Adding support for ssh using kubectl port-forward to access k8s instance (#2412) * Add sshjump support. * Update lcm script. - add comments - rename variables - typo * Set imagePullPolicy to IfNotPresent. * add support for port-forward * remove unused * comments * Disable ControlMaster for ssh_options_list * nit * update to disable rest of the ControlMaster * command runner rsync update * relocating run_on_k8s * relocate run_on_k8s * Make Kubernetes specific env variables available when joining a cluster via SSH * merge k8s_cloud_beta1 * format * remove redundant utils.py * format and comments * update with proxy_to_k8s * Update sky/authentication.py Co-authored-by: Romil Bhardwaj <[email protected]> * resolving comments on structures * Update sky/utils/command_runner.py Co-authored-by: Romil Bhardwaj <[email protected]> * document on nodeport/port-forward proxycommand * error handling when socat is not installed * removing KUBECONFIG from port-forward shell script * nit * nit * Add suport for nodeport * Update sky/utils/kubernetes_utils.py Co-authored-by: Romil Bhardwaj <[email protected]> * update * switch svc when conflicting jump pod svc exist * format * Update sky/utils/kubernetes_utils.py Co-authored-by: Romil Bhardwaj <[email protected]> * refactoring check for socat * resolve comments * add ServiceType enum and port-forward proxy script * update k8s env var access * add check for container status remove unused func * nit * update get_external_ip for portforward mode * conditionally use sudo and quote values of env var --------- Co-authored-by: Avi Weit <[email protected]> Co-authored-by: hemildesai <[email protected]> Co-authored-by: Romil Bhardwaj <[email protected]> * refactor * fix * updates * lint * Update sky/skylet/providers/kubernetes/node_provider.py * fix test * [k8s] Showing reasons for provisioning failure in K8s (#2422) * surface provision failure message * nit * nit * format * nit * CPU message fix * update Insufficient memory handling * nit * nit * Update sky/skylet/providers/kubernetes/node_provider.py Co-authored-by: Romil Bhardwaj <[email protected]> * Update sky/skylet/providers/kubernetes/node_provider.py Co-authored-by: Romil Bhardwaj <[email protected]> * Update sky/skylet/providers/kubernetes/node_provider.py Co-authored-by: Romil Bhardwaj <[email protected]> * Update sky/skylet/providers/kubernetes/node_provider.py Co-authored-by: Romil Bhardwaj <[email protected]> * format * update gpu failure message and condition * fix GPU handling cases * fix * comment * nit * add try except block with general error handling --------- Co-authored-by: Romil Bhardwaj <[email protected]> * cleanup * lint * fix for ssh jump image_id * comments * ssh jump refactor * lint * image build fixes --------- Co-authored-by: Avi Weit <[email protected]> Co-authored-by: Hemil Desai <[email protected]> Co-authored-by: Doyoung Kim <[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.
Adds a class method to
Cloudcalledquery_env_varswhich can query cloud specific env vars. Addresses #2287Tested (run the relevant ones):
bash format.shsky launch -- printenvand verified thatKUBERNETES_SERVICE_PORTetc are includedpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_namebash tests/backward_comaptibility_tests.sh