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.

Sign up for GitHub

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

Jump to bottom

[Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES #47445

Merged
merged 3 commits into from
Sep 18, 2024
Merged

[Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES #47445

merged 3 commits into from
Sep 18, 2024

Conversation

MortalHappiness
Copy link
Member

Why are these changes needed?

Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES behaviors in the following PRs:

Related issue number

N/A

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@MortalHappiness
Copy link
Member Author

@kevin85421 @jjyao @andrewsykim PTAL

@MortalHappiness MortalHappiness changed the title [Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES [Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES Sep 1, 2024
@MortalHappiness MortalHappiness changed the title [Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES [Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_CR_AFTER_JOB_FINISHES Sep 1, 2024
@MortalHappiness
Copy link
Member Author

Doc build link: https://anyscale-ray--47445.com.readthedocs.build/en/47445/cluster/kubernetes/getting-started/rayjob-quick-start.html

@kevin85421 kevin85421 self-assigned this Sep 2, 2024
@kevin85421 kevin85421 added the kuberay Issues for the Ray/Kuberay integration that are tracked on the Ray side label Sep 3, 2024
@anyscalesam anyscalesam added P1 Issue that should be fixed within a few weeks core Issues that should be addressed in Ray Core labels Sep 3, 2024
kevin85421
kevin85421 approved these changes Sep 4, 2024
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Can you add some comments indicating which version started supporting these APIs?

doc/source/cluster/kubernetes/getting-started/rayjob-quick-start.md Outdated
* Automatic resource cleanup
* `shutdownAfterJobFinishes` (Optional): Determines whether to recycle the RayCluster after the Ray job finishes. The default value is false.
* `ttlSecondsAfterFinished` (Optional): Only works if `shutdownAfterJobFinishes` is true. The KubeRay operator deletes the RayCluster and the submitter `ttlSecondsAfterFinished` seconds after the Ray job finishes. The default value is 0.
* `activeDeadlineSeconds` (Optional): If the RayJob doesn't transition the `JobDeploymentStatus` to `Complete` or `Failed` within `activeDeadlineSeconds`, the KubeRay operator transitions the `JobDeploymentStatus` to `Failed`, citing `DeadlineExceeded` as the reason.
* `DELETE_RAYJOB_CR_AFTER_JOB_FINISHES` (Optional): If this environment variable is set to true, the RayJob custom resource itself will be deleted if `shutdownAfterJobFinishes` is also set to true. Note that all resources created by the RayJob will be deleted, including the K8s Job.
Copy link
Member

Choose a reason for hiding this comment

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

I think this environment variable should be set in the KubeRay operator, not the CR. Could we clarify that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@MortalHappiness MortalHappiness force-pushed the docs/update-rayjob-backoff-delete branch 2 times, most recently from 69e9af2 to d215030 Compare September 4, 2024 16:46
andrewsykim
andrewsykim reviewed Sep 5, 2024
View reviewed changes
doc/source/cluster/kubernetes/getting-started/rayjob-quick-start.md Outdated
* Automatic resource cleanup
* `shutdownAfterJobFinishes` (Optional): Determines whether to recycle the RayCluster after the Ray job finishes. The default value is false.
* `ttlSecondsAfterFinished` (Optional): Only works if `shutdownAfterJobFinishes` is true. The KubeRay operator deletes the RayCluster and the submitter `ttlSecondsAfterFinished` seconds after the Ray job finishes. The default value is 0.
* `activeDeadlineSeconds` (Optional): If the RayJob doesn't transition the `JobDeploymentStatus` to `Complete` or `Failed` within `activeDeadlineSeconds`, the KubeRay operator transitions the `JobDeploymentStatus` to `Failed`, citing `DeadlineExceeded` as the reason.
* `DELETE_RAYJOB_CR_AFTER_JOB_FINISHES` (Optional, added in version 1.2.0): This environment variable should be set for the KubeRay operator, not the RayJob resource. If this environment variable is set to true, the RayJob custom resource itself will be deleted if `shutdownAfterJobFinishes` is also set to true. Note that all resources created by the RayJob will be deleted, including the K8s Job.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it still confusing to add DELETE_RAYJOB_CR_AFTER_JOB_FINISHES here, because the other entries are for RayJob API where-as this is an environment variable in kuberay.

Maybe it's time for a dedicated section in the kubray installation docs about environment variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. But I don't find a good place to put the kuberay-related configuration. @kevin85421 @andrewsykim where do you recommend to put them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not a blocker for this PR, but we should consider a "Configuring KubeRay" page. There are other enviornment variables that are not documented yet

Copy link
Member

Choose a reason for hiding this comment

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

open an issue ray-project/kuberay#2356 to track the progress.

@kevin85421 kevin85421 mentioned this pull request Sep 5, 2024
Open
2 tasks
angelinalg
angelinalg approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@angelinalg angelinalg left a comment

Choose a reason for hiding this comment

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

Just some style nits. Please consider using Vale to find these issues in the future. Please excuse any inaccuracies I introduced in my suggestions and correct as needed. Happy to answer any questions you have about the suggestions. Thanks for your contribution!

doc/source/cluster/kubernetes/getting-started/rayjob-quick-start.md Outdated
* Automatic resource cleanup
* `shutdownAfterJobFinishes` (Optional): Determines whether to recycle the RayCluster after the Ray job finishes. The default value is false.
* `ttlSecondsAfterFinished` (Optional): Only works if `shutdownAfterJobFinishes` is true. The KubeRay operator deletes the RayCluster and the submitter `ttlSecondsAfterFinished` seconds after the Ray job finishes. The default value is 0.
* `activeDeadlineSeconds` (Optional): If the RayJob doesn't transition the `JobDeploymentStatus` to `Complete` or `Failed` within `activeDeadlineSeconds`, the KubeRay operator transitions the `JobDeploymentStatus` to `Failed`, citing `DeadlineExceeded` as the reason.
* `DELETE_RAYJOB_CR_AFTER_JOB_FINISHES` (Optional, added in version 1.2.0): This environment variable should be set for the KubeRay operator, not the RayJob resource. If this environment variable is set to true, the RayJob custom resource itself will be deleted if `shutdownAfterJobFinishes` is also set to true. Note that all resources created by the RayJob will be deleted, including the K8s Job.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `DELETE_RAYJOB_CR_AFTER_JOB_FINISHES` (Optional, added in version 1.2.0): This environment variable should be set for the KubeRay operator, not the RayJob resource. If this environment variable is set to true, the RayJob custom resource itself will be deleted if `shutdownAfterJobFinishes` is also set to true. Note that all resources created by the RayJob will be deleted, including the K8s Job.
* `DELETE_RAYJOB_CR_AFTER_JOB_FINISHES` (Optional, added in version 1.2.0): Set this environment variable for the KubeRay operator, not the RayJob resource. If you set this environment variable to true, the RayJob custom resource itself is deleted if you also set `shutdownAfterJobFinishes` to true. Note that KubeRay deletes all resources created by the RayJob, including the Kubernetes Job.

Copy link
Member Author

@MortalHappiness MortalHappiness Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I thought I have no style error because here says that the editorial style is enforced in CI by Vale and the CI passed without error. I've pushed updates. Is the CI broken so that it did not catch this error?

MortalHappiness and others added 3 commits September 10, 2024 20:35
@MortalHappiness
[Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_…
a05fdaa 
…CR_AFTER_JOB_FINISHES

Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@kevin85421 @MortalHappiness
Update doc/source/cluster/kubernetes/getting-started/rayjob-quick-sta…
f77e1f9 
…rt.md

Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
@MortalHappiness
[Style][Docs] Fix vale error
780d9cd 
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@kevin85421 kevin85421 added the go add ONLY when ready to merge, run all tests label Sep 17, 2024
@jjyao jjyao merged commit 265dcc6 into ray-project:master Sep 18, 2024
6 checks passed
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@MortalHappiness @kevin85421 @ujjawal-khare
[Docs][KubeRay] Update RayJob doc for backoffLimit and DELETE_RAYJOB_…
da4c6fb 
…CR_AFTER_JOB_FINISHES (ray-project#47445)

Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
Co-authored-by: Kai-Hsun Chen <kaihsun@apache.org>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsykim andrewsykim andrewsykim left review comments

@kevin85421 kevin85421 kevin85421 approved these changes

@angelinalg angelinalg angelinalg approved these changes

@architkulkarni architkulkarni Awaiting requested review from architkulkarni

@maxpumperla maxpumperla Awaiting requested review from maxpumperla maxpumperla is a code owner

@pcmoritz pcmoritz Awaiting requested review from pcmoritz pcmoritz is a code owner

Assignees

@kevin85421 kevin85421

Labels
core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests kuberay Issues for the Ray/Kuberay integration that are tracked on the Ray side P1 Issue that should be fixed within a few weeks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MortalHappiness @andrewsykim @kevin85421 @angelinalg @jjyao @anyscalesam