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.

Sign up for GitHub

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

Jump to bottom

[Datasets] [Docs] Update map_batches documentation #28435

Merged
merged 25 commits into from
Sep 27, 2022
Merged

[Datasets] [Docs] Update map_batches documentation #28435

merged 25 commits into from
Sep 27, 2022

Conversation

bveeramani
Copy link
Member

@bveeramani bveeramani commented Sep 11, 2022 •
edited
Loading

Signed-off-by: Balaji Veeramani balaji@anyscale.com

Depends on:

Why are these changes needed?

  • Users aren't aware of BatchMapper
  • Users can't figure out how to write UDFs, and there's no link to the relevant user guide
  • Users get confused by the examples

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

clarkzinzow
clarkzinzow reviewed Sep 12, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

Nice, big improvement!

python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated 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 Sep 13, 2022
@bveeramani bveeramani removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Sep 15, 2022
clarkzinzow
clarkzinzow approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

This is a massive improvement to this docstring! Only small nits left, so this looks good to merge after the default batch PR is merged and this is rebased.

python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated 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 Sep 21, 2022
@bveeramani bveeramani removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Sep 22, 2022
jianoaix
jianoaix reviewed Sep 22, 2022
View reviewed changes
python/ray/data/dataset.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py
Call this function to iterate over batches of data.

:meth:`~Dataset.default_batch_format`
Call this function to determine the default batch type.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: to learn? User cannot choose default type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think "determine" is okay in this context.

This is one of the Google definitions:

ascertain or establish exactly, typically as a result of research or calculation.
"the point of our study was to determine what is true, not what is practicable"

python/ray/data/dataset.py Outdated 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 Sep 23, 2022
@bveeramani bveeramani removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Sep 23, 2022
@clarkzinzow clarkzinzow merged commit b233222 into ray-project:master Sep 27, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
@bveeramani @WeichenXu123
[Datasets] [Docs] Update map_batches documentation (ray-project#28435)
85ac94f 
- Users aren't aware of BatchMapper
- Users can't figure out how to write UDFs, and there's no link to the relevant user guide
- Users get confused by the examples

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericl ericl ericl left review comments

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@shawnpanda shawnpanda shawnpanda left review comments

@xwjiang2010 xwjiang2010 xwjiang2010 left review comments

@jianoaix jianoaix jianoaix approved these changes

@scv119 scv119 Awaiting requested review from scv119

@jjyao jjyao Awaiting requested review from jjyao

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@c21 c21 Awaiting requested review from c21

Assignees

@ericl ericl

@clarkzinzow clarkzinzow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bveeramani @ericl @clarkzinzow @shawnpanda @xwjiang2010 @jianoaix