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

[UX] default to minimal logging (no module/line number/timestamp). #3980

Merged
merged 7 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
2 changes: 1 addition & 1 deletion sky/execution.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ def _execute(
#
# Disable the usage collection for this status command.
env = dict(os.environ,
**{env_options.Options.DISABLE_LOGGING.value: '1'})
**{str(env_options.Options.DISABLE_LOGGING): '1'})
subprocess_utils.run(
'sky status --no-show-managed-jobs --no-show-services', env=env)
print()
Expand Down
7 changes: 3 additions & 4 deletions sky/optimizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -965,10 +965,10 @@ def _print_candidates(node_to_candidate_map: _TaskToPerCloudCandidates):
f'Multiple {cloud} instances satisfy '
f'{acc_name}:{int(acc_count)}. '
f'The cheapest {candidate_list[0]!r} is considered '
f'among:\n{instance_list}.\n')
f'among:\n{instance_list}.')
if is_multi_instances:
logger.info(
f'To list more details, run \'sky show-gpus {acc_name}\'.')
f'To list more details, run: sky show-gpus {acc_name}\n')

@staticmethod
def _optimize_dag(
Expand Down Expand Up @@ -1101,8 +1101,7 @@ def ordinal_number(n):
Optimizer.print_optimized_plan(graph, topo_order, best_plan,
total_time, total_cost,
node_to_cost_map, minimize_cost)
if not env_options.Options.MINIMIZE_LOGGING.get():
Optimizer._print_candidates(local_node_to_candidate_map)
Optimizer._print_candidates(local_node_to_candidate_map)
return best_plan


Expand Down
4 changes: 2 additions & 2 deletions sky/utils/controller_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -362,15 +362,15 @@ def shared_controller_vars_to_fill(
'sky_python_cmd': constants.SKY_PYTHON_CMD,
}
env_vars: Dict[str, str] = {
env.value: '1' for env in env_options.Options if env.get()
str(env): str(int(env.get())) for env in env_options.Options
}
env_vars.update({
# Should not use $USER here, as that env var can be empty when
# running in a container.
constants.USER_ENV_VAR: getpass.getuser(),
constants.USER_ID_ENV_VAR: common_utils.get_user_hash(),
# Skip cloud identity check to avoid the overhead.
env_options.Options.SKIP_CLOUD_IDENTITY_CHECK.value: '1',
str(env_options.Options.SKIP_CLOUD_IDENTITY_CHECK): '1',
})
if skypilot_config.loaded():
# Only set the SKYPILOT_CONFIG env var if the user has a config file.
Expand Down
22 changes: 16 additions & 6 deletions sky/utils/env_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,27 @@

class Options(enum.Enum):
"""Environment variables for SkyPilot."""
IS_DEVELOPER = 'SKYPILOT_DEV'
SHOW_DEBUG_INFO = 'SKYPILOT_DEBUG'
DISABLE_LOGGING = 'SKYPILOT_DISABLE_USAGE_COLLECTION'
MINIMIZE_LOGGING = 'SKYPILOT_MINIMIZE_LOGGING'

# (env var name, default value)
IS_DEVELOPER = ('SKYPILOT_DEV', False)
SHOW_DEBUG_INFO = ('SKYPILOT_DEBUG', False)
DISABLE_LOGGING = ('SKYPILOT_DISABLE_USAGE_COLLECTION', False)
MINIMIZE_LOGGING = ('SKYPILOT_MINIMIZE_LOGGING', True)
Copy link
Collaborator

@Michaelvll Michaelvll Sep 25, 2024

Choose a reason for hiding this comment

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

We should still show the stamps and file name/lines when SKYPILOT_DEBUG is set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, done. PTAL.

# Internal: this is used to skip the cloud user identity check, which is
# used to protect cluster operations in a multi-identity scenario.
# Currently, this is only used in the job and serve controller, as there
# will not be multiple identities, and skipping the check can increase
# robustness.
SKIP_CLOUD_IDENTITY_CHECK = 'SKYPILOT_SKIP_CLOUD_IDENTITY_CHECK'
SKIP_CLOUD_IDENTITY_CHECK = ('SKYPILOT_SKIP_CLOUD_IDENTITY_CHECK', False)

def __init__(self, env_var, default):
concretevitamin marked this conversation as resolved.
Show resolved Hide resolved
self.env_var = env_var
self.default = default

def __repr__(self) -> str:
return self.env_var

def get(self):
concretevitamin marked this conversation as resolved.
Show resolved Hide resolved
"""Check if an environment variable is set to True."""
return os.getenv(self.value, 'False').lower() in ('true', '1')
return os.getenv(self.env_var,
str(self.default)).lower() in ('true', '1')
Loading