-
Notifications
You must be signed in to change notification settings - Fork 7k
[Test][KubeRay] Add doctest for RayCluster Quickstart doc #51249
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
[Test][KubeRay] Add doctest for RayCluster Quickstart doc #51249
Conversation
a4820f9 to
c077002
Compare
a365e4f to
15318f7
Compare
|
cc @rueian Would you mind taking the first pass at the review? |
doc/source/cluster/kubernetes/getting-started/raycluster-quick-start.ipynb
Outdated
Show resolved
Hide resolved
Signed-off-by: Chi-Sheng Liu <[email protected]>
Signed-off-by: Chi-Sheng Liu <[email protected]>
15318f7 to
a713127
Compare
doc/source/cluster/kubernetes/getting-started/raycluster-quick-start.ipynb
Outdated
Show resolved
Hide resolved
Signed-off-by: Chi-Sheng Liu <[email protected]>
kevin85421
left a comment
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.
Nice!
ci/k8s/run-kuberay-doc-tests.sh
Outdated
| echo "--- Install Python dependencies" | ||
| pip install -c python/requirements_compiled.txt pytest nbval bash_kernel | ||
| python -m bash_kernel.install | ||
| pip install "ray[default]==2.43.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.
It's better to use the same version as the Ray images in the Ray documentation.
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.
Updated to 2.41.0
| replace: KUBERAY-OPERATOR-POD-NAME | ||
|
|
||
| [raycluster-head-pod-name] | ||
| regex: raycluster-kuberay-workergroup-worker-[a-z0-9]{5} |
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.
For KubeRay v1.4.0, the naming changes a bit. This may require some updates in the future.
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.
Yes we need to go back to change this doc after 1.4.0 release. But for now let's keep the naming here.
| "cell_type": "markdown", | ||
| "id": "2248a2d8-bd70-4ac3-8fa8-7beeb4a2a8ae", | ||
| "metadata": { | ||
| "collapsed": true, |
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.
what does this mean?
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.
Actually, I don't know. Maybe some hidden cells or metadata added by Jupyter Notebook? I edited this file with Jupyter Lab and didn't notice this cell. But it's not important since it doesn't show up in the doc or affect the tests.
| @@ -0,0 +1,42 @@ | |||
| (kuberay-operator-deploy)= | |||
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 doc requires doc team to review cc @angelinalg.
Signed-off-by: Chi-Sheng Liu <[email protected]>
jjyao
left a comment
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.
Need doc team review. cc @dayshah
|
|
||
| echo "--- Run doc tests" | ||
| cd doc/source/cluster/kubernetes | ||
| py.test --nbval getting-started/raycluster-quick-start.ipynb --nbval-kernel-name bash --sanitize-with doc_sanitize.cfg |
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.
What's the test plan for kuberay doc test? Are we going to manually add each doc file to this?
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.
Currently, yes. I haven't found a good way to identify which files contain doc tests yet. Maybe we can search all .ipynb files under the current directory and check whether their kernel is bash_kernel to find those that contain doc tests.
…t#51249) Signed-off-by: Chi-Sheng Liu <[email protected]>
…t#51249) Signed-off-by: Chi-Sheng Liu <[email protected]> Signed-off-by: Dhakshin Suriakannu <[email protected]>
Signed-off-by: Chi-Sheng Liu <[email protected]> Signed-off-by: Srinath Krishnamachari <[email protected]>
Why are these changes needed?
Real implementation for #50988. Add doc test for the "RayCluster Quickstart" doc.
doc links:
Related issue number
ray-project/kuberay#3157
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.