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

fix: llama stack build use UV_SYSTEM_PYTHON to install dependencies to system environment #1163

Merged
merged 8 commits into from
Feb 20, 2025

Conversation

yanxi0830
Copy link
Contributor

@yanxi0830 yanxi0830 commented Feb 20, 2025

What does this PR do?

image

This PR

Use UV_SYSTEM_PYTHON to make sure dependencies are installed in current system environment. Which will be used in the Colab environment.

UV_SYSTEM_PYTHON=1 llama stack build --template together --image-type venv

Test Plan

  • Works in Colab environment
image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Feb 20, 2025
@yanxi0830 yanxi0830 marked this pull request as ready for review February 20, 2025 03:19
@yanxi0830 yanxi0830 changed the title fix: llama stack build add --system-install flag to add to current env fix: llama stack build add --system-install flag to install dependencies to current python environment Feb 20, 2025
@yanxi0830 yanxi0830 added this to the v0.1.4 milestone Feb 20, 2025
Copy link
Contributor

@SLR722 SLR722 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 the quick fix!

Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

Hm, I wonder if we really need this additional complexity again. A proposal: if we just base our operation on the UV_SYSTEM_PYTHON env variable? If that is set, we know we don't intend to create a separate venv -- this is the variable uv has settled upon and we already use it in the notebook example?

@yanxi0830 yanxi0830 changed the title fix: llama stack build add --system-install flag to install dependencies to current python environment fix: llama stack build use UV_SYSTEM_PYTHON to install dependencies to current python environment Feb 20, 2025
@yanxi0830 yanxi0830 changed the title fix: llama stack build use UV_SYSTEM_PYTHON to install dependencies to current python environment fix: llama stack build use UV_SYSTEM_PYTHON to install dependencies to system environment Feb 20, 2025
@yanxi0830
Copy link
Contributor Author

Hm, I wonder if we really need this additional complexity again. A proposal: if we just base our operation on the UV_SYSTEM_PYTHON env variable? If that is set, we know we don't intend to create a separate venv -- this is the variable uv has settled upon and we already use it in the notebook example?

ugh my bad, just realized I've been using UV_SYSTEM_PROMPT to test and hence added the flag. updated the script now

@ashwinb
Copy link
Contributor

ashwinb commented Feb 20, 2025

Also, please update the PR summary also!

@yanxi0830 yanxi0830 merged commit 61f43b8 into main Feb 20, 2025
5 checks passed
@yanxi0830 yanxi0830 deleted the system-install-flag branch February 20, 2025 06:21
@yanxi0830 yanxi0830 linked an issue Feb 20, 2025 that may be closed by this pull request
2 tasks
@@ -73,11 +73,16 @@ run() {
local env_name="$1"
local pip_dependencies="$2"
local special_pip_deps="$3"

if [ -n "${UV_SYSTEM_PYTHON:-}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

oh I missed this, but I think you need to initialize this before the set -euo pipefail otherwise you will get a failure if this env var is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the ${UV_SYSTEM_PYTHON:-} part will cover it, but just to be consistent: ca687d3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colab notebook broken with dependencies missing
4 participants