-
Notifications
You must be signed in to change notification settings - Fork 550
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
chore: create alias to source sky env in dev mode #4553
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to the SkyPilot community @sachiniyer! Thanks for this PR - left a minor comment.
echo 'export SKYPILOT_DEV=1' >> ~/.bashrc | ||
{% endif %} | ||
|
||
{% if controller_envs.get("SKYPILOT_DEV") %} | ||
type sky-env > /dev/null 2>&1 || echo 'alias sky-env="{{ sky_activate_python_env }}"' >> ~/.bashrc | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the is_dev
conditional check above and wrap it into the same block?
echo 'export SKYPILOT_DEV=1' >> ~/.bashrc | |
{% endif %} | |
{% if controller_envs.get("SKYPILOT_DEV") %} | |
type sky-env > /dev/null 2>&1 || echo 'alias sky-env="{{ sky_activate_python_env }}"' >> ~/.bashrc | |
{% endif %} | |
echo 'export SKYPILOT_DEV=1' >> ~/.bashrc | |
type sky-env > /dev/null 2>&1 || echo 'alias sky-env="{{ sky_activate_python_env }}"' >> ~/.bashrc | |
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, IIUC type
checks symbols in the active shell. While it will work here, do you think something like this would be a more robust check:
if ! grep -q "alias sky-env=" ~/.bashrc; then
echo 'alias sky-env="{{ sky_activate_python_env }}"' >> ~/.bashrc
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the grep solution, especially since I don't think it's guaranteed that the commands here are running in bash (could be some other sh like dash)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both suggestions make sense. Let me try again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I was able to get the grep statement to work.
However, I am not able to figure out how the other dev statement ({% if is_dev %}
) works. I am also not seeing 'export SKYPILOT_DEV=1'
in .bashrc
when I run the smoke tests or when I create a job manually (with SKYPILOT_DEV=1
set in my local shell)
I am also not seeing is_dev
in the vars_to_fill
map here. Not sure if there is something I am missing.
I also know that the controller envs are being set into the environment through this statement
envs:
{%- for env_name, env_value in controller_envs.items() %}
{{env_name}}: {{env_value}}
{%- endfor %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, yeah, that's apparently been broken for more than a year. #2458 removed is_dev
but apparently didn't remove this use. Good catch!
Could you move the export SKYPILOT_DEV=1
into your new conditional, delete the old one, and add a similar grep check? e.g.
grep -q "export SKYPILOT_DEV=" ~/.bashrc || echo 'export SKYPILOT_DEV=1' >> ~/.bashrc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the wait (work taking up a bit of my time). I've made the changes and ran a quick sanity check - seems to be good.
I am realizing it would be good to refactor this to a list of aliases outside of the jinja template, but I'll leave that for a future PR.
025d719
to
d3fcafa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sachiniyer! Left a comment on correctness.
{% if is_dev %} | ||
# Internal: disable logging for manually logging into the jobs controller for debugging. | ||
echo 'export SKYPILOT_DEV=1' >> ~/.bashrc | ||
{% if controller_envs.get("SKYPILOT_DEV") %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to update this if conditional to check the actual value of SKYPILOT_DEV.
We set this key as:
env_vars: Dict[str, str] = {
env.env_key: str(int(env.get())) for env in env_options.Options
}
the str type cast will always return a true here. To validate, I ran export SKYPILOT_DEBUG=0; sky jobs launch -- echo hi
and my controller indeed had SKYPILOT_DEV=1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, definitely missed that (and my bad, I should have checked the negative case).
I'll do a string match against the value, and perform two tests (and post results)
- Positive case with
SKYPILOT_DEBUG=1
- Negative case with
SKYPILOT_DEBUG=0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Positive Case
(skypilot) 👋🌍 finished/skypilot git:(master) ❱❱ export SKYPILOT_DEV=1
(skypilot) 👋🌍 finished/skypilot git:(master) ❱❱ pyenv exec sky jobs launch -n test-1 --cloud aws examples/managed_job.yaml -y -d
(base) ubuntu@ip-172-31-28-229:~$ echo $SKYPILOT_DEV
1
(base) ubuntu@ip-172-31-28-229:~$
Negative Case
(skypilot) 👋🌍 finished/skypilot git:(master) ❱❱ export SKYPILOT_DEV=0
(skypilot) 👋🌍 finished/skypilot git:(master) ❱❱ pyenv exec sky jobs launch -n test-1 --cloud aws examples/managed_job.yaml -y -d
(base) ubuntu@ip-172-31-28-216:~$ echo $SKYPILOT_DEV
(base) ubuntu@ip-172-31-28-216:~$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sachiniyer! LGTM.
{% if is_dev %} | ||
# Internal: disable logging for manually logging into the jobs controller for debugging. | ||
echo 'export SKYPILOT_DEV=1' >> ~/.bashrc | ||
{% if controller_envs.get("SKYPILOT_DEV") != "0" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we generally use single quotes for strs in our codebase
Added a small change to the template to create an alias for sourcing the python env. Addressing #4453 as a good first issue.
I ran the smoke tests, as well as verified that the alias existed when I created a job.
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_job_and_serve
conda deactivate; bash -i tests/backward_compatibility_tests.sh