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

[Data] Deprecate Dataset.get_internal_block_refs() #46455

Merged
merged 29 commits into from
Jul 11, 2024
Merged

[Data] Deprecate Dataset.get_internal_block_refs() #46455

merged 29 commits into from
Jul 11, 2024

Conversation

scottjlee
Copy link
Contributor

@scottjlee scottjlee commented Jul 6, 2024 •
edited
Loading

Why are these changes needed?

Stacked on: #46369 (merged)

Replaces Dataset.get_internal_block_refs() usages with Dataset.iter_internal_ref_bundles(), and marks the method as deprecated.

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

scottjlee and others added 16 commits July 1, 2024 23:23
@scottjlee
add iter_internal_block_refs
c313a52 
Signed-off-by: sjl <[email protected]>
@scottjlee
Merge branch 'master' into 0701-count
09b905d
@scottjlee
return iterator over refbundles
5ef8041 
Signed-off-by: sjl <[email protected]>
@scottjlee
Merge branch '0701-count' of https://github.com/scottjlee/ray into 07…
172e423 
…01-count
@scottjlee
fix docs
374898b 
Signed-off-by: sjl <[email protected]>
@scottjlee
update consumption api usage
b0ec894 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
fix
f0b49f1 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
clean up
aa368d4 
Signed-off-by: sjl <[email protected]>
@scottjlee
comments
ab8d6ed 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
comments
128614d 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
lint
65c3f4e 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
Merge branch 'master' into 0701-count
7956f35
@scottjlee
fix tests
958d306 
Signed-off-by: sjl <[email protected]>
@scottjlee
Merge branch '0701-count' of https://github.com/scottjlee/ray into 07…
d6a1d79 
…01-count

Signed-off-by: sjl <[email protected]>
@scottjlee
update tests
87151ee 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee
replace get_internal_block_refs usages
6d15acd 
Signed-off-by: Scott Lee <[email protected]>
@scottjlee scottjlee added the go add ONLY when ready to merge, run all tests label Jul 6, 2024
@scottjlee scottjlee self-assigned this Jul 8, 2024
@scottjlee scottjlee marked this pull request as ready for review July 10, 2024 21:50
@scottjlee scottjlee assigned omatthew98 and bveeramani and unassigned scottjlee Jul 10, 2024
scottjlee
scottjlee commented Jul 10, 2024
View reviewed changes
python/ray/data/_internal/execution/interfaces/ref_bundle.py
@@ -59,7 +59,7 @@ def __setattr__(self, key, value):
object.__setattr__(self, key, value)

@property
def block_refs(self) -> List[BlockMetadata]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix incorrect typehint

Copy link
Member

Choose a reason for hiding this comment

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

Nice

omatthew98
omatthew98 approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@omatthew98 omatthew98 left a comment

Choose a reason for hiding this comment

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

Don't have a lot of context here but the code changes seem good. One small nit on the comment.

python/ray/data/_internal/execution/interfaces/ref_bundle.py Outdated
def _ref_bundles_iterator_to_block_refs_list(
ref_bundles: Iterator[RefBundle],
) -> List[ObjectRef[Block]]:
"""Convert an iterator of RefBundles to a list of object references to Blocks."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Change to "Convert an iterator of RefBundles to a list of Block object references." to avoid the double to, for a sec thought this was a double transformation.

bveeramani
bveeramani approved these changes Jul 11, 2024
View reviewed changes
python/ray/data/tests/test_zip.py Outdated
@@ -69,7 +69,8 @@ def test_zip_different_num_blocks_split_smallest(
override_num_blocks=num_blocks2,
)
ds = ds1.zip(ds2).materialize()
num_blocks = len(ds.get_internal_block_refs())
bundles = ds.iter_internal_ref_bundles()
num_blocks = sum([len(b.block_refs) for b in bundles])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
num_blocks = sum([len(b.block_refs) for b in bundles])
num_blocks = sum(len(b.block_refs) for b in bundles)

python/ray/data/tests/test_size_estimation.py Outdated
assert nblocks == 1, nblocks
ctx.target_max_block_size = 2_000_000
nblocks = len(ds2.map_batches(lambda x: x, batch_size=16).get_internal_block_refs())
bundles = ds2.map_batches(lambda x: x, batch_size=16).iter_internal_ref_bundles()
nblocks = sum([len(b.block_refs) for b in bundles])
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Here and elsewhere

Suggested change
nblocks = sum([len(b.block_refs) for b in bundles])
nblocks = sum(len(b.block_refs) for b in bundles)

@scottjlee scottjlee mentioned this pull request Jul 11, 2024
Merged
9 tasks
@bveeramani bveeramani merged commit 19dc58e into ray-project:master Jul 11, 2024
5 checks passed
Catch-Bull pushed a commit to Catch-Bull/ray that referenced this pull request Jul 15, 2024
@scottjlee
[Data] Deprecate Dataset.get_internal_block_refs() (ray-project#46455)
71b6135 
Replaces Dataset.get_internal_block_refs() usages with Dataset.iter_internal_ref_bundles(), and marks the method as deprecated.

Signed-off-by: sjl <[email protected]>
Signed-off-by: Scott Lee <[email protected]>
Signed-off-by: hejialing.hjl <[email protected]>
bveeramani pushed a commit that referenced this pull request Jul 16, 2024
@scottjlee
[Data] Update ExecutionPlan.execute_to_iterator() to return `RefBun…
1b0af29 
…dles` instead of `(Block, BlockMetadata)` (#46575)

Followup to #46369 and
#46455.
Update `ExecutionPlan.execute_to_iterator()` to return `RefBundles`
instead of `(Block, BlockMetadata)`, to unify the logic between
`RefBundle`s and `Block`s. Also refactor the `iter_batches()` code path
accordingly to handle `RefBundle`s instead of raw `Block` and
`BlockMetadata`.

Signed-off-by: sjl <[email protected]>
Signed-off-by: Scott Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omatthew98 omatthew98 omatthew98 approved these changes

@bveeramani bveeramani bveeramani approved these changes

@ericl ericl Awaiting requested review from ericl ericl is a code owner

@scv119 scv119 Awaiting requested review from scv119 scv119 is a code owner

@c21 c21 Awaiting requested review from c21 c21 is a code owner

@amogkam amogkam Awaiting requested review from amogkam amogkam is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang stephanie-wang is a code owner

Assignees

@omatthew98 omatthew98

@bveeramani bveeramani

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@scottjlee @omatthew98 @bveeramani