-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[docs] yarn update #6173
[docs] yarn update #6173
Conversation
Co-Authored-By: Edward Oakes <ed.nmi.oakes@gmail.com>
Test PASSed. |
Test PASSed. |
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.
Overall these updates look good to me.
doc/yarn/ray-skein.yaml
Outdated
# This activates a pre-existing environment for dependency management. | ||
source /home/rayonyarn/miniconda3/bin/activate | ||
# This obtains the Skein Application ID which is used when pushing addresses to worker services. | ||
APP_ID=$(python -c 'import skein;print(skein.properties.application_id)') | ||
|
||
# This register the Ray head addresses needed by the workers with the Skein key-value store. |
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.
This reads a little off. Perhaps:
This stores the Ray head address in the Skein key-value store, so the workers can retrieve it later
# # doesn't require any specific way of distributing files, but this | ||
# # is a good one for python projects. | ||
# # See https://jcrist.github.io/skein/distributing-files.html | ||
# environment: environment.tar.gz |
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.
Unfortunately github diff commenting only lets me comment on changes, so I'll add this here
This section is copied around in a few places, and may be confusing for new readers as it's double commented out, and not really explained. I'd either drop it, or alter the language to make it clearer that including a packaged environment file is optional, and if they wish to do so they can do so here in this way.
Also, you may want to remove the code block from https://github.com/ray-project/ray/pull/6173/files#diff-de6110281d163f755df4b298ab7b2bc6R71 and use a literalinclude
to the example file instead, not sure.
doc/source/deploy-on-yarn.rst
Outdated
|
||
Obtain the Skein Application ID which is used when pushing addresses to worker services. | ||
Register the Ray head addresses needed by the workers in the Skein key-value store. |
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.
Is there more than one address? Otherwise you might want "address" instead of "addresses".
Test FAILed. |
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.