Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[core] Support generators to allow tasks to return a dynamic number of objects #28291
[core] Support generators to allow tasks to return a dynamic number of objects #28291
Changes from 18 commits
5636eb9
3457a46
031640b
e59dd65
775ff3e
b316b9f
2d0f5d4
faf64cb
5d92940
7c69b32
c1ecf26
422e4b6
62b427c
636954c
0b73319
450d428
a0bca43
6511a04
7ea1404
6e4663a
7bba5b7
c31b1c4
167c6be
44d9d85
39ba48f
805a088
3360d63
2ed252a
3a811dc
285d98d
7a86f19
b045d34
7b3cf27
92a8491
492b95e
07ca73d
c196c67
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hmm, I thought the return value of dynamic generator task is also an
ObjectRef
that we can get the generator by callingray.get()
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 was a change that I made to try to make the semantics closer to the usual list of ObjectRefs.
The main thing that might be a bit weird is that there's an implicit ray.get when you try to access the iterator. But it's fine to pass it around to other tasks before then.
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.
I preferred this syntax over returning an
ObjectRef
directly, though, since it makes it explicit that anum_returns="dynamic"
task does not return a normalObjectRef
.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.
Isn't this limiting though? For example, if you wanted to wait for two generators to be completed, now you couldn't use ray.wait.
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.
You could still use
ray.wait
like this as long as we support passing ObjectRefGenerators too:I think the only thing that wouldn't be supported is if you wanted to block on the results of both at the same time with the list syntax:
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 has implications for other APIs as well right? (e.g., ray.kill, passing as args). It seems at the least this would change the type signature to Union[ObjectRef, ObjectRefGenerator] for many APIs.
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.
I'm wondering whether we should use
-1
instead of"dynamic"
. This way it will be more friendly to other static type languages like Java.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.
I don't think the syntax here affects what syntax we use in Java. Prefer "dynamic" since it's more clear than -1.