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] Use original Python path for SkyPilot runtime #3326

Merged
merged 12 commits into from
Mar 19, 2024
Merged

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Mar 18, 2024

Fixes #3324

master (823999a)
for i in {1..5}; do time sky launch -c test-gcp-$i --cloud gcp --cpus 2 -y; done

1m59.195s
2m0.108s
1m56.507s
1m54.553s
1m54.242s

mean: 1m56.921s

This PR (39daf35):
for i in {1..5}; do time sky launch -c test-gcp-$i --cloud gcp --cpus 2 -y; done

1m53.456s
2m7.647s
1m55.891s
1m59.496s
2m7.193s

mean: 2m0.7366s (3.8156s overhead)

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky launch --cloud azure -c test-azure --cpus 2+ echo hi
    • sky launch -c test-env task.yaml --cloud gcp
      resources:
        cpus: 2+
      
      setup: |
        conda activate myenv
        if [ $? -ne 0 ]; then
          conda create -n myenv python=3.10 -y
          conda activate myenv
        fi
      
        grep -qxF 'conda activate myenv' ~/.bashrc || echo "conda activate myenv" >> ~/.bashrc
      
      run: |
        echo hi
        echo bye
  • All smoke tests: pytest tests/test_smoke.py
  • pytest tests/test_smoke.py --aws
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@Michaelvll Michaelvll marked this pull request as ready for review March 18, 2024 10:00
Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

Thanks @Michaelvll!

Wondering if we should test against the following. This change invokes <fixed path to ray/sky> under arbitrary user-created conda env. Let's say

  • Ray/Sky depend on some library X requiring certain version range
  • User conda env removed this library (or changed its version to be outside the range)

Then, does this change still work?

We may need to run all smoke tests too.

@Michaelvll
Copy link
Collaborator Author

This change invokes <fixed path to ray/sky> under arbitrary user-created conda env

We are invoking the ray/sky in the base or the default env when the VM is just created, instead of the user-created conda env.

Just tested with the following yaml and it works well:

resources:
  cpus: 2+

setup: |
  conda activate myenv
  if [ $? -ne 0 ]; then
    conda create -n myenv python=3.7 -y
    conda activate myenv
  fi

  grep -qxF 'conda activate myenv' ~/.bashrc || echo "conda activate myenv" >> ~/.bashrc
  pip install ray==2.6.0

run: |
  conda env list
  echo hi
  echo bye

Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

Thanks @Michaelvll! Do we need to run some back compat tests too?

sky/backends/backend_utils.py Outdated Show resolved Hide resolved
sky/execution.py Outdated Show resolved Hide resolved
@@ -27,7 +27,8 @@ def restart_skylet():


proc = subprocess.run(
'ps aux | grep -v "grep" | grep "sky.skylet.skylet" | grep "python3 -m"',
f'ps aux | grep -v "grep" | grep "sky.skylet.skylet" | '
f'grep "{constants.SKY_PYTHON_CMD} -m"',
Copy link
Member

Choose a reason for hiding this comment

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

Why not grep this too on L16?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

L16 is for backward compatibility, as we need to make sure that the skylet started before this PR (will not have the full path to the python executable) will be correctly killed.

sky/skylet/constants.py Outdated Show resolved Hide resolved
sky/skylet/log_lib.py Outdated Show resolved Hide resolved
sky/skylet/constants.py Show resolved Hide resolved
Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Michaelvll!

sky/backends/backend_utils.py Outdated Show resolved Hide resolved
@Michaelvll Michaelvll merged commit c180b2b into master Mar 19, 2024
19 checks passed
@Michaelvll Michaelvll deleted the use-python-path branch March 19, 2024 16:45
Michaelvll added a commit that referenced this pull request Mar 20, 2024
@Michaelvll Michaelvll restored the use-python-path branch March 20, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] User changing default env will make job submission fail
2 participants