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

[Core][2/3] Make streaming generator public API #38784 #41436

Merged

Conversation

rkooo567
Copy link
Contributor

Why are these changes needed?

This is the second PR of 3 steps.

All sort of implementation work [done]
Rename StreamingObjectRefGenerator and annotate it as a public API (and move it to init.py) <---- this PR
Typing support [future]

This PR renames StreamingObjectRefGenerator - > ObjectRefGenerator. This also means the existing dynamic generator's (num_returns="dynamic") return type will become ObjectRefGenerator -> DynamicObjectRefGenerator, which is a breaking change, but we will anyway deprecate this API. I am open for a different name if we'd like to keep the perfect backward compatibility.

Related issue number

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

doc/source/serve/model_composition.md Outdated Show resolved Hide resolved
@@ -544,9 +544,9 @@ def _submit_task(
"""Submit the task with index task_idx.

NOTE: When dynamic block splitting is enabled, returns
Tuple[ObjectRef[ObjectRefGenerator], None] instead of
Tuple[ObjectRef[DynamicObjectRefGenerator], None] instead of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @raulchen is this still relevant with data?

Copy link
Contributor

Choose a reason for hiding this comment

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

this code path is being deprecated.

@rkooo567
Copy link
Contributor Author

@pcmoritz @ericl this PR will have a breaking change to num_returns="dynamic" because it changes the name of the class to ObjectRefGenerator -> DynamicObjectRefGenerator, so that ObjectRefGenerator can be used for the streaming generator. (we are going to deprecate num_returns="dynamic" as we discussed in the public API proposal)

If this is concerning, we can instead use ObjRefGenerator for num_returns="streaming" instead. Let me know if you guys prefer this option.

Copy link
Contributor

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

If I remember correctly, we said that dynamic is experimental so it should be fine?

@@ -233,7 +233,7 @@ def _send_query_java(self, query: Query) -> ray.ObjectRef:

def _send_query_python(
self, query: Query
) -> Union[ray.ObjectRef, "ray._raylet.StreamingObjectRefGenerator"]:
) -> Union[ray.ObjectRef, "ray._raylet.ObjectRefGenerator"]:
"""Send the query to a Python replica."""
if query.metadata.is_streaming:
method = self._actor_handle.handle_request_streaming.options(
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can/should remove the num_returns="streaming" here

doc/source/ray-core/ray-generator.rst Outdated Show resolved Hide resolved
doc/source/ray-core/ray-generator.rst Outdated Show resolved Hide resolved
doc/source/ray-core/ray-generator.rst Outdated Show resolved Hide resolved
doc/source/ray-core/ray-generator.rst Outdated Show resolved Hide resolved
doc/source/ray-core/ray-generator.rst Outdated Show resolved Hide resolved
doc/source/serve/model_composition.md Outdated Show resolved Hide resolved
python/ray/_raylet.pyx Outdated Show resolved Hide resolved
python/ray/_raylet.pyx Outdated Show resolved Hide resolved
python/ray/_raylet.pyx Outdated Show resolved Hide resolved
src/ray/protobuf/common.proto Show resolved Hide resolved
@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 30, 2023
@rkooo567 rkooo567 merged commit fbbc4be into ray-project:master Dec 4, 2023
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants