[Datasets] Avoid unnecessary reads when truncating a dataset with ds.limit()
#27343
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
clarkzinzow
requested review from
ericl,
scv119,
jjyao and
jianoaix
as code owners
clarkzinzow
force-pushed
the
datasets/fix/limit-no-redundant-reads
branch
from
b18b99f
to
7c0b974
Compare
|
Nice, we should make all transform APIs real and not run as consumption APIs. |
jianoaix
reviewed
Aug 2, 2022
jianoaix
reviewed
Aug 3, 2022
| @@ -112,6 +112,30 @@ def split_by_bytes(self, bytes_per_split: int) -> List["BlockList"]: | |||
| ) | |||
| return output | |||
|
|
|||
| def truncate_by_rows(self, limit: int) -> "BlockList": | |||
There was a problem hiding this comment.
It looks truncate_by_blocks, since this is working at block level with constraint to cover desired num of rows.
There was a problem hiding this comment.
We're truncating to the number of rows given, similar to above split_by_bytes where we're splitting by the number of bytes given.
There was a problem hiding this comment.
Ok. I think one thing important is that we don't split block in order to make up the desired bytes or rows. I don't have a good naming suggestion though (maybe just compensate it with comments).
There was a problem hiding this comment.
That's already indicated in the docstring so I think it should be fine.
clarkzinzow
force-pushed
the
datasets/fix/limit-no-redundant-reads
branch
from
d3847a0
to
3dc9eee
Compare
jianoaix
approved these changes
Aug 5, 2022
| @@ -112,6 +112,30 @@ def split_by_bytes(self, bytes_per_split: int) -> List["BlockList"]: | |||
| ) | |||
| return output | |||
|
|
|||
| def truncate_by_rows(self, limit: int) -> "BlockList": | |||
There was a problem hiding this comment.
Ok. I think one thing important is that we don't split block in order to make up the desired bytes or rows. I don't have a good naming suggestion though (maybe just compensate it with comments).
clarkzinzow
added a commit
to clarkzinzow/ray
that referenced
this pull request
Aug 5, 2022
….limit()` (ray-project#27343) Datasets currently eagerly kicks off all read tasks when truncating a dataset immediately after a read via ray.data.read_*().limit(); this results in a lot of wasted computation and unnecessary object store bloat, especially when trying to poke at a very small subset of the data. This PR avoids these unnecessary reads by truncating the blocklist to the minimum number of blocks needed to meet the row limit before doing the actual block splitting, thereby avoiding materialization of unnecessary read tasks in the common splitting path.
scv119
pushed a commit
that referenced
this pull request
Aug 6, 2022
…datasets with `.limit()` (#27585) * [Datasets] Avoid unnecessary reads when truncating a dataset with `ds.limit()` (#27343) Datasets currently eagerly kicks off all read tasks when truncating a dataset immediately after a read via ray.data.read_*().limit(); this results in a lot of wasted computation and unnecessary object store bloat, especially when trying to poke at a very small subset of the data. This PR avoids these unnecessary reads by truncating the blocklist to the minimum number of blocks needed to meet the row limit before doing the actual block splitting, thereby avoiding materialization of unnecessary read tasks in the common splitting path. * [Datasets] [Docs] Improve `.limit()` and `.take()` docstrings (#27367) Improve docstrings for .limit() and .take(), making the distinction more clear. Signed-off-by: Clark Zinzow <[email protected]>
Stefan-1313
pushed a commit
to Stefan-1313/ray_mod
that referenced
this pull request
Aug 18, 2022
….limit()` (ray-project#27343) Datasets currently eagerly kicks off all read tasks when truncating a dataset immediately after a read via ray.data.read_*().limit(); this results in a lot of wasted computation and unnecessary object store bloat, especially when trying to poke at a very small subset of the data. This PR avoids these unnecessary reads by truncating the blocklist to the minimum number of blocks needed to meet the row limit before doing the actual block splitting, thereby avoiding materialization of unnecessary read tasks in the common splitting path. Signed-off-by: Stefan van der Kleij <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Datasets currently eagerly kicks off all read tasks when truncating a dataset immediately after a read via
ray.data.read_*().limit(); this results in a lot of wasted computation and unnecessary object store bloat, especially when trying to poke at a very small subset of the data.This PR avoids these unnecessary reads by truncating the blocklist to the minimum number of blocks needed to meet the row limit before doing the actual block splitting, thereby avoiding materialization of unnecessary read tasks in the common splitting path.
Related issue number
Closes #27340
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.