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

Add support for env files. #2296

Merged
merged 4 commits into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions sky/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"""
import copy
import datetime
from dotenv import dotenv_values
Copy link
Member

Choose a reason for hiding this comment

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

nit: we use google styleguide where 3rd party libs are imported in 2nd block, L42-45. Also from styleguide: import dotenv and use dotenv.dotenv_values below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

import functools
import multiprocessing
import os
Expand Down Expand Up @@ -330,6 +331,15 @@ def _parse_env_var(env_var: str) -> Tuple[str, str]:
'or KEY.')
return ret[0], ret[1]

def _merge_env_vars(env_dict: Optional[Dict[str, str]],
env_list: List[Tuple[str, str]]) -> List[Tuple[str, str]]:
"""Merges all values from env_list into env_dict, overwriting any old values."""
if not env_dict:
return env_list
for (key, value) in env_list:
env_dict[key] = value
return list(env_dict.items())


_TASK_OPTIONS = [
click.option('--name',
Expand Down Expand Up @@ -382,6 +392,17 @@ def _parse_env_var(env_var: str) -> Tuple[str, str]:
default=None,
help=('Custom image id for launching the instances. '
'Passing "none" resets the config.')),
click.option(
'--env-file',
required=False,
type=dotenv_values,
Copy link
Member

Choose a reason for hiding this comment

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

nit: dotenv.dotenv_values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

help="""\
Path to a dotenv file with environment variables to set on the remote
node.

If any values from ``--env-file`` conflict with values set by
``--env``, the ``--env`` value will be preferred."""
),
click.option(
'--env',
required=False,
Expand Down Expand Up @@ -1308,6 +1329,7 @@ def launch(
use_spot: Optional[bool],
image_id: Optional[str],
env: List[Tuple[str, str]],
env_file: Optional[Dict[str, str]],
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
env: List[Tuple[str, str]],
env_file: Optional[Dict[str, str]],
env_file: Optional[Dict[str, str]],
env: List[Tuple[str, str]],

nit: Would be nice to keep the order in _TASK_OPTIONS and here to be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

disk_size: Optional[int],
disk_tier: Optional[str],
idle_minutes_to_autostop: Optional[int],
Expand All @@ -1326,6 +1348,7 @@ def launch(
In both cases, the commands are run under the task's workdir (if specified)
and they undergo job queue scheduling.
"""
env = _merge_env_vars(env_file, env)
backend_utils.check_cluster_name_not_reserved(
cluster, operation_str='Launching tasks on it')
if backend_name is None:
Expand Down Expand Up @@ -1424,6 +1447,7 @@ def exec(
use_spot: Optional[bool],
image_id: Optional[str],
env: List[Tuple[str, str]],
env_file: Optional[Dict[str, str]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

):
# NOTE(dev): Keep the docstring consistent between the Python API and CLI.
"""Execute a task or a command on a cluster (skip setup).
Expand Down Expand Up @@ -1483,6 +1507,7 @@ def exec(
sky exec mycluster --env WANDB_API_KEY python train_gpu.py

"""
env = _merge_env_vars(env_file, env)
backend_utils.check_cluster_name_not_reserved(
cluster, operation_str='Executing task on it')
handle = global_user_state.get_handle_from_cluster_name(cluster)
Expand Down Expand Up @@ -3460,6 +3485,7 @@ def spot_launch(
image_id: Optional[str],
spot_recovery: Optional[str],
env: List[Tuple[str, str]],
env_file: Optional[Dict[str, str]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

disk_size: Optional[int],
disk_tier: Optional[str],
detach_run: bool,
Expand All @@ -3480,6 +3506,7 @@ def spot_launch(

sky spot launch 'echo hello!'
"""
env = _merge_env_vars(env_file, env)
task_or_dag = _make_task_or_dag_from_entrypoint_with_overrides(
entrypoint,
name=name,
Expand Down Expand Up @@ -3908,6 +3935,7 @@ def benchmark_launch(
use_spot: Optional[bool],
image_id: Optional[str],
env: List[Tuple[str, str]],
env_file: Optional[Dict[str, str]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

disk_size: Optional[int],
disk_tier: Optional[str],
idle_minutes_to_autostop: Optional[int],
Expand All @@ -3920,6 +3948,7 @@ def benchmark_launch(
Alternatively, specify the benchmarking resources in your YAML (see doc),
which allows benchmarking on many more resource fields.
"""
env = _merge_env_vars(env_file, env)
record = benchmark_state.get_benchmark_from_name(benchmark)
if record is not None:
raise click.BadParameter(f'Benchmark {benchmark} already exists. '
Expand Down
1 change: 1 addition & 0 deletions sky/setup_files/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def parse_readme(readme: str) -> str:
# PrettyTable with version >=2.0.0 is required for the support of
# `add_rows` method.
'PrettyTable>=2.0.0',
'python-dotenv==1.0.0',
Copy link
Member

Choose a reason for hiding this comment

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

Can we relax this? This requires >= 3.8. Even though 3.7 is end of life now, it seems removing this will install 0.21.1 and it still works for the use case (tested).

» pip install python-dotenv==1.0.0      130 ↵
ERROR: Ignored the following versions that require a different python version: 1.0.0 Requires-Python >=3.8
ERROR: Could not find a version that satisfies the requirement python-dotenv==1.0.0 (from versions: 0.1.0, 0.1.2, 0.1.3, 0.1.5, 0.2.0, 0.3.0, 0.4.0, 0.5.0, 0.5.1, 0.6.0, 0.6.1, 0.6.2, 0.6.3, 0.6.4, 0.6.5, 0.7.0, 0.7.1, 0.8.0, 0.8.1, 0.8.2, 0.9.0, 0.9.1, 0.10.0, 0.10.1, 0.10.2, 0.10.3, 0.10.4, 0.10.5, 0.11.0, 0.12.0, 0.13.0, 0.14.0, 0.15.0, 0.16.0, 0.17.0, 0.17.1, 0.18.0, 0.19.0, 0.19.1, 0.19.2, 0.20.0, 0.21.0, 0.21.1)
ERROR: No matching distribution found for python-dotenv==1.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

# Lower version of ray will cause dependency conflict for
# click/grpcio/protobuf.
'ray[default]>=2.2.0,<=2.4.0',
Expand Down
Loading