-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[Doc] Streaming generator alpha doc #39914
[Doc] Streaming generator alpha doc #39914
Conversation
@@ -124,9 +124,9 @@ | |||
# Special mocking of packaging.version.Version is required when using sphinx; | |||
# we can't just add this to autodoc_mock_imports, as packaging is imported by | |||
# sphinx even before it can be mocked. Instead, we patch it here. | |||
import packaging | |||
import packaging.version as packaging_version |
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 found this is broken when using Python > 3.8 because you cannot just access packaging.verison
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.
Really confused why this hasn't come up before. I just checked and indeed if I run python and try
import packaging
packaging.version.Version
I get an error. But if I run the existing documentation builder (with python 3.11) there's no error. Maybe some package is doing something to make this compatible? I really don't understand this.
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 guess packaging version could also matter. I think depending on how you import "version", you cannot access it via packaging.version, but only via from package import version
========== | ||
.. warning:: | ||
|
||
``num_returns="dynamic"`` :ref:`generator API <dynamic_generators>` is soft deprecated from Ray 2.8 due to its :ref:`limitation <dynamic-generators-limitation>` and hard deprecated from Ray 2.9. |
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.
We have a task to deprecate this API in 2.8
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.
Nice find with the packaging
issue. Looks good to me! 🚢
@@ -124,9 +124,9 @@ | |||
# Special mocking of packaging.version.Version is required when using sphinx; | |||
# we can't just add this to autodoc_mock_imports, as packaging is imported by | |||
# sphinx even before it can be mocked. Instead, we patch it here. | |||
import packaging | |||
import packaging.version as packaging_version |
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.
Really confused why this hasn't come up before. I just checked and indeed if I run python and try
import packaging
packaging.version.Version
I get an error. But if I run the existing documentation builder (with python 3.11) there's no error. Maybe some package is doing something to make this compatible? I really don't understand this.
============== | ||
|
||
`Python generators <https://docs.python.org/3/howto/functional.html#generators>`_ are functions | ||
that behave like an iterator, yielding one value per iteration. Ray also supports the generators API. |
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.
that behave like an iterator, yielding one value per iteration. Ray also supports the generators API. | |
that behave like iterators, yielding one value per iteration. Ray also supports the generators API. |
With a Ray generator, the caller can access the object refeference | ||
before the task ``f`` finishes. | ||
|
||
**Ray generator is useful when** |
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.
We should also talk about when this is not preferred?
Basically help user decide whether to use static num_returns or streaming num_returns.
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 should we add a new section? I feel like it should be a pattern / anti-pattern doc rather than this doc?
cc @angelinalg I believe the comments are all handled |
Why are these changes needed?
This is a PR to write a streaming generator documentation. Unlike existing generator doc, streaming generator doc is under "advanced" usage, and I will add proper links from each task/actor/pattern pages to this doc.
The doc is written with the assumption is is an "alpha" feature. It is important to merge this doc PR before proceeding with #38784 as it will need to refer docs here and there. Some of the contents (such as "API is subject to change") will be removed after we merge these PRs.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.