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

Make ray.get(timeout=0) to throw timeout error #30210

Closed
wants to merge 4 commits into from

Conversation

vitrioil
Copy link
Contributor

Why are these changes needed?

As mentioned in the issue it makes it consistent with other .get call.

However, I wonder if this change might be unexpected for existing code where 0 is already used without expecting an error.

Related issue number

Closes #28465

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

@vitrioil vitrioil marked this pull request as ready for review November 13, 2022 16:00
@vitrioil
Copy link
Contributor Author

@krfricke any idea who can review this? (I think the build failures are unrelated?)

@krfricke
Copy link
Contributor

I think the main question is if this was intended API before or not. But yes, this will effectively be an API change and it will be hard to provide a graceful transition.

Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@krfricke
Copy link
Contributor

After internal discussion, it seems like this was a bug and we should fix it. cc @rickyyx to take a look from Ray core side

Copy link
Contributor

@rickyyx rickyyx left a comment

Choose a reason for hiding this comment

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


via GIPHY

Should we also piggyback doc improvmeent?

Args:
object_refs: Object ref of the object to get or a list of object refs
to get.
timeout (Optional[float]): The maximum amount of time in seconds to
wait before returning.
Returns:
A Python object or a list of Python objects.
Raises:
GetTimeoutError: A GetTimeoutError is raised if a timeout is set and
the get takes longer than timeout to return.
Exception: An exception is raised if the task that created the object
or that created one of the objects raised an exception.

I feel the current documentation is not explicit in the behaviour of timeout=0 vs timeout=None, which could be better.

Also, does it make sense to change these as well now we have a proper timeout=0 behaviour:
https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/ray-project/ray%24+ray.get.*timeout%3D0.1&patternType=standard&sm=1

@rkooo567
Copy link
Contributor

We shouldn't always raise an error? I think it should get the object if the object is ready. Is this covered?

Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

request change on the comment ^

@krfricke
Copy link
Contributor

@vitrioil let me know if you want to take these changes, otherwise I'm happy to add them to this PR later this week

@rickyyx
Copy link
Contributor

rickyyx commented Dec 15, 2022

An update on this - since this change will be breaking backward compatibility (there might be users who have code depends on this "buggy" behaviour), we will take a slower migration approach.

  1. We will collect some signals on existing usage of ray.get(timeout=0)
    a. telemetry data
    b. Ask in slack and OSS community
  2. [Optional] add warnings of changes in the future release.
    a. This step will depend on if there's significant usage of the timeout=0 behaviour.
  3. Perform the migration (this PR) in future releases.

@rickyyx rickyyx added the do-not-merge Do not merge this PR! label Dec 15, 2022
@stale
Copy link

stale bot commented Jan 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jan 15, 2023
@krfricke
Copy link
Contributor

This is still relevant. Can we make the stale bot less aggressive?

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jan 17, 2023
@stale
Copy link

stale bot commented Feb 18, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Feb 18, 2023
@rickyyx
Copy link
Contributor

rickyyx commented Feb 22, 2023

Keep.

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Feb 22, 2023
@stale
Copy link

stale bot commented Mar 24, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 24, 2023
@stale
Copy link

stale bot commented Apr 11, 2023

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

@stale stale bot closed this Apr 11, 2023
@rkooo567
Copy link
Contributor

This will be done by #28465 in ray 2.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Do not merge this PR! stale The issue is stale. It will be closed within 7 days unless there are further conversation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] ray.get(future, timeout=0) blocks forever but should return immediately
6 participants