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

[Core] Fix quote in command runner #3572

Merged
merged 7 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 2 additions & 3 deletions sky/backends/cloud_vm_ray_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -3175,7 +3175,8 @@ def _setup_node(node_id: int) -> None:
f'{create_script_code} && {setup_cmd}',
log_path=setup_log_path,
process_stream=False,
source_bashrc=True,
# We do not source bashrc for setup, since bashrc is sourced
# in the script already.
)

def error_message() -> str:
Expand Down Expand Up @@ -3724,7 +3725,6 @@ def tail_managed_job_logs(self,
process_stream=False,
ssh_mode=command_runner.SshMode.INTERACTIVE,
stdin=subprocess.DEVNULL,
source_bashrc=True,
)

def tail_serve_logs(self, handle: CloudVmRayResourceHandle,
Expand Down Expand Up @@ -3762,7 +3762,6 @@ def tail_serve_logs(self, handle: CloudVmRayResourceHandle,
process_stream=False,
ssh_mode=command_runner.SshMode.INTERACTIVE,
stdin=subprocess.DEVNULL,
source_bashrc=True,
)

def teardown_no_lock(self,
Expand Down
4 changes: 2 additions & 2 deletions sky/provision/instance_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ def _setup_node(runner: command_runner.CommandRunner, log_path: str):
stream_logs=False,
log_path=log_path,
require_outputs=True,
# Installing depencies requires source bashrc to access the PATH
# in bashrc.
# Installing dependencies requires source bashrc to access
# conda.
source_bashrc=True)
retry_cnt = 0
while returncode == 255 and retry_cnt < _MAX_RETRY:
Expand Down
11 changes: 7 additions & 4 deletions sky/skylet/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,18 @@
# AWS's Deep Learning AMI's default conda environment.
CONDA_INSTALLATION_COMMANDS = (
'which conda > /dev/null 2>&1 || '
'(wget -nc https://repo.anaconda.com/miniconda/Miniconda3-py310_23.11.0-2-Linux-x86_64.sh -O Miniconda3-Linux-x86_64.sh && ' # pylint: disable=line-too-long
'{ wget -nc https://repo.anaconda.com/miniconda/Miniconda3-py310_23.11.0-2-Linux-x86_64.sh -O Miniconda3-Linux-x86_64.sh && ' # pylint: disable=line-too-long
'bash Miniconda3-Linux-x86_64.sh -b && '
'eval "$(~/miniconda3/bin/conda shell.bash hook)" && conda init && '
'conda config --set auto_activate_base true); '
'grep "# >>> conda initialize >>>" ~/.bashrc || conda init;'
'conda config --set auto_activate_base true && '
# Use $(echo ~) instead of ~ to avoid the error "no such file or directory".
# Also, not using $HOME to avoid the error HOME variable not set.
f'echo "$(echo ~)/miniconda3/bin/python" > {SKY_PYTHON_PATH_FILE}; }}; '
'grep "# >>> conda initialize >>>" ~/.bashrc || '
'{ conda init && source ~/.bashrc; };'
'(type -a python | grep -q python3) || '
'echo \'alias python=python3\' >> ~/.bashrc;'
'(type -a pip | grep -q pip3) || echo \'alias pip=pip3\' >> ~/.bashrc;'
'source ~/.bashrc;'
# Writes Python path to file if it does not exist or the file is empty.
f'[ -s {SKY_PYTHON_PATH_FILE} ] || which python3 > {SKY_PYTHON_PATH_FILE};')

Expand Down
10 changes: 6 additions & 4 deletions sky/utils/command_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ def _get_command_to_run(
# cluster by 1 second.
# sourcing ~/.bashrc is not required for internal executions
command += [
'true && export OMP_NUM_THREADS=1 PYTHONWARNINGS=ignore'
f' && ({cmd})'
shlex.quote('true && export OMP_NUM_THREADS=1 '
f'PYTHONWARNINGS=ignore && ({cmd})')
]
if not separate_stderr:
command.append('2>&1')
Expand Down Expand Up @@ -431,10 +431,12 @@ def run(
cmd,
process_stream,
separate_stderr,
# A hack to remove the following bash warnings (twice):
# A hack to remove the following SSH warning+bash warnings (twice):
# Warning: Permanently added 'xx.xx.xx.xx' to the list of known...
# bash: cannot set terminal process group
# bash: no job control in this shell
skip_lines=5 if source_bashrc else 0,
# When not source_bashrc, the bash warning will only show once.
skip_lines=5 if source_bashrc else 3,
source_bashrc=source_bashrc)
command = base_ssh_command + [shlex.quote(command_str)]

Expand Down
Loading