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] More robust conda setup when python 3.12 is in the image #4035

Merged
merged 11 commits into from
Feb 22, 2025

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Oct 4, 2024

Not urgent, but this makes the python path for the case where python 3.12 is the default python in the custom image.

Performance tests

On default image

Ran for 5 times

Master

===> multitime results
1: sky launch -y --cpus 2 --cloud aws
            Mean        Std.Dev.    Min         Median      Max
real        79.540      29.046      59.366      63.620      136.207     
user        3.890       0.392       3.574       3.690       4.646       
sys         0.178       0.026       0.153       0.162       0.219 

Current PR

1: sky launch -y --cpus 2 --cloud aws
            Mean        Std.Dev.    Min         Median      Max
real        62.031      2.946       56.946      63.365      65.049      
user        3.968       0.155       3.774       3.964       4.235       
sys         0.313       0.073       0.236       0.290       0.442 

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • Test on an image with python 3.12 as default python version and check performance difference
      sky launch --image-id docker:ubuntu --cpus 2 --cloud aws
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@Michaelvll Michaelvll marked this pull request as ready for review October 14, 2024 00:38
'echo "Creating conda env with Python 3.10" && '
f'conda create -y -n {SKY_REMOTE_PYTHON_ENV_NAME} python=3.10 && '
f'conda activate {SKY_REMOTE_PYTHON_ENV_NAME};'
f'PYTHON_EXEC=$(conda run -n {SKY_REMOTE_PYTHON_ENV_NAME} which python3); '
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the main place that we get the correct path for python3

@Michaelvll Michaelvll requested a review from cblmemo October 31, 2024 07:01
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @Michaelvll ! A quick question - do we need to change the python path file to $PYTHON_EXEC as well?

@cg505
Copy link
Collaborator

cg505 commented Dec 19, 2024

Cool, I didn't know uv venv had this feature! Maybe this is too ambitious, but do we need conda at all if we can do this?

@Michaelvll
Copy link
Collaborator Author

Michaelvll commented Dec 19, 2024

Cool, I didn't know uv venv had this feature! Maybe this is too ambitious, but do we need conda at all if we can do this?

Yep, we should gradually get rid of conda. As we have move off the dependency of conda for our internal dependencies, the main purpose of conda now is just to make sure that a user gets the same python env when they start machines on any cloud and can do conda commands.

The next step would be:
Completely move to uv, add uv as the recommended way to install packages in the machine in SkyPilot doc, and add a flag to install conda if people need it.

Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

Sounds good, ty. We should do smoke tests and the testing plan you mention in the PR description.

@Michaelvll
Copy link
Collaborator Author

A user said this PR resolves their issue with a image with python 3.12 as the base python

@Michaelvll
Copy link
Collaborator Author

There seems no significant performance difference. Merging this PR after the smoke tests passed.

@Michaelvll
Copy link
Collaborator Author

/smoke-test --aws

@Michaelvll
Copy link
Collaborator Author

/quicktest-core

@Michaelvll
Copy link
Collaborator Author

/smoke-test --aws -k test_job_queue_with_docker

@Michaelvll
Copy link
Collaborator Author

/smoke-test --aws -k test_managed_jobs_pipeline_recovery_aws

@Michaelvll
Copy link
Collaborator Author

/smoke-test --aws -k test_managed_jobs_retry_logs

@Michaelvll Michaelvll merged commit d430ac4 into master Feb 22, 2025
19 checks passed
@Michaelvll Michaelvll deleted the more-robust-conda-setup branch February 22, 2025 18:27
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.

3 participants