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.

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

[workflow]align the behavior of workflow's max_retires with remote function's max_retries #22903

Merged
merged 6 commits into from
Mar 24, 2022

Conversation

lchu-ibm
Copy link
Contributor

@lchu-ibm lchu-ibm commented Mar 8, 2022

Why are these changes needed?

To address the issue #22824

Basically the current behavior of max_retries in workflow is different from the one in remote functions in the following ways:

  1. workflow's max_retries is not the number of retries, but the number of total tries.
  2. workflow's max_retries does not allow "-1" (infinite retries) while remote function's max_retries does.

This PR altered the behavior of max_retries in workflow to be consistent with the max_retries in remote functions:

  1. make max_retries to be truly max retries (i.e. total tries = original try + max retries)
  • implementation
  • update logging
  • update tests
  1. make max_retries accept infinite tries (i.e. max_retries=-1)

Related issue number

#22824

Checks

  • 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 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 :(

@fishbone
Copy link
Contributor

Overall LGTM. Some comments there.

@fishbone fishbone added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Mar 23, 2022
@fishbone fishbone removed the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Mar 23, 2022
Copy link
Contributor

@fishbone fishbone left a comment

Choose a reason for hiding this comment

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

LG!

@fishbone fishbone merged commit 63d6884 into ray-project:master Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants