-
Notifications
You must be signed in to change notification settings - Fork 485
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
[Spot] Add spot job name in the SKYPILOT_TASK_ID
env var
#3424
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.
Thanks @Michaelvll, some questions.
docs/source/examples/spot-jobs.rst
Outdated
.. note:: | ||
|
||
The :code:`$SKYPILOT_TASK_ID` environment variable is also available in the :code:`run` section of each task. It is unique for each task in the pipeline. | ||
For the :code:`eval` task, the :code:`$SKYPILOT_TASK_ID` will be: |
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.
One UX discussion: I found myself reading these as "spot_id". Took a while to realize that it should be read as "_spot" and appended with "id-xxxx". Shall we skip the _id
part if backward compatible?
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.
Oh, it should be read as spot_id
and appended with _<job_id>-<task_id>
. I guess there will be no significant backward compatibility error in the case. I am not strongly for keeping the id
or not. Wdyt?
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.
Hmm, reason I found "spot_id" odd is that typically we can just append the IDs without saying IDs first. (E.g., we prepend the timestamp without saying timestamp-
.)
In this case, how about we remove -id
? I guess moving spot-id
as a whole may be less desirable.
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.
We do have an _id
for normal cluster job. To keep it align, we may have to remove it for those jobs as well.
Removed is for both now.
docs/source/examples/spot-jobs.rst
Outdated
.. note:: | ||
|
||
The :code:`$SKYPILOT_TASK_ID` environment variable is also available in the :code:`run` section of each task. It is unique for each task in the pipeline. | ||
For the :code:`eval` task, the :code:`$SKYPILOT_TASK_ID` will be: |
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.
Hmm, reason I found "spot_id" odd is that typically we can just append the IDs without saying IDs first. (E.g., we prepend the timestamp without saying timestamp-
.)
In this case, how about we remove -id
? I guess moving spot-id
as a whole may be less desirable.
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
…to task-name-in-id
I updated the naming of |
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.
LGTM, thanks @Michaelvll.
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
…to task-name-in-id
Fixes #3391
This PR adds the task name in the
SKYPILOT_TASK_ID
.Tested (run the relevant ones):
bash format.sh
sky spot launch -n test "echo \$SKYPILOT_TASK_ID" --cpus 2
sky launch -c my-clus -n test "echo \$SKYPILOT_TASK_ID" --cpus 2
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh