[data] Adding in updated code to from uris release test by omatthew98 · Pull Request #56091 · ray-project/ray

omatthew98 · 2025-08-29T18:59:17Z

Why are these changes needed?

Update the from uris release test to use the new code from #55824.

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

Signed-off-by: Matthew Owen <mowen@anyscale.com>

gemini-code-assist

Code Review

This pull request updates the read_from_uris benchmark to use the new ray.data.expressions.download API. The change is a good simplification, but I've found a critical performance issue due to the removal of dataset repartitioning, which would cause the benchmark to run serially. I've also added a minor suggestion to improve code conciseness. Please review the detailed comments.

release/nightly_tests/dataset/read_from_uris_benchmark.py

gemini-code-assist · 2025-08-29T19:00:24Z

release/nightly_tests/dataset/read_from_uris_benchmark.py

+        images = []
+        for b in batch["image_bytes"]:
+            image = Image.open(io.BytesIO(b)).convert("RGB")
+            images.append(np.array(image))


This for-loop can be written more concisely and idiomatically using a list comprehension, which improves readability.

Suggested change

images = []

for b in batch["image_bytes"]:

image = Image.open(io.BytesIO(b)).convert("RGB")

images.append(np.array(image))

images = [

np.array(Image.open(io.BytesIO(b)).convert("RGB"))

for b in batch["image_bytes"]

]

Signed-off-by: Matthew Owen <mowen@anyscale.com>

iamjustinhsu · 2025-08-29T23:13:45Z

release/nightly_tests/dataset/read_from_uris_benchmark.py

+        return table.set_column(table.schema.get_field_index("key"), "key", new_col)
+
+    ds = metadata.map_batches(convert_key, batch_format="pyarrow")
+    ds = ds.with_column("image_bytes", download("key"))


why is batch_format necessary?

It isn't explicitly necessary, I could rewrite the function to use default/numpy batches. The function I already was testing with used PyArrow batches in the convert key function so specifying the batches is necessary to work with that.

Signed-off-by: Matthew Owen <mowen@anyscale.com>

omatthew98 · 2025-09-02T22:14:55Z

Test Results Updated Test in PR:

Fixed size: 229.85s (build)
Autoscaling: 500.54s (build)

Test Results Latest Master:

Fixed size: 1283.89s (build)
Autoscaling: 1334.39s (build)

bveeramani · 2025-09-03T16:36:18Z

release/nightly_tests/dataset/read_from_uris_benchmark.py

+
+    ds = metadata.map_batches(convert_key, batch_format="pyarrow")
+    ds = ds.with_column("image_bytes", download("key"))
+    ds = ds.map_batches(decode_images)


Nit: i think this might be simpler as map

Agree that might be simpler, but my thought was when we have the decode API we should be applying that with a map batches similar to this so better to update the test to match that pattern.

bveeramani · 2025-09-03T16:42:11Z

python/ray/data/_internal/planner/plan_download_op.py

        compute_strategy=partition_compute,  # Use actor-based compute for callable class
        ray_remote_args=op._ray_remote_args,
        ray_remote_args_fn=op._ray_remote_args_fn,
+        supports_fusion=False,  # Prevent fusion with upstream operations that use different compute strategies


This is a drive-by bug fix?

Yeah, upstream operators could fuse with the download operator and it was causing an assertion to fail.

was the previous supports_fusion=True also causing performance issues?

This assertion was failing

File "/home/ray/anaconda3/lib/python3.12/site-packages/ray/data/_internal/planner/plan_udf_map_op.py", line 382, in _wrapped_udf_map_fn assert ray.data._map_actor_context is not None

so the code could not run / there wasn't really any performance to compare.

oh i see this wasn't a test

@omatthew98 is there a more holistic fix we should make? Like, is disabling fusion the best move long-term?

Confused why this happens in this case but not with regular map operator fusion.

Signed-off-by: Matthew Owen <mowen@anyscale.com>

omatthew98 · 2025-09-04T22:13:02Z

python/ray/data/_internal/logical/operators/map_operator.py

        ray_remote_args: Optional[Dict[str, Any]] = None,
    ):
-        super().__init__("Download", input_op, ray_remote_args=ray_remote_args)
+        from ray.data._internal.compute import ActorPoolStrategy


@bveeramani Added a better fix here. TLDR by not specifying the compute as we initialize the logical operator it defaults to the TaskPoolStrategy for compute causing us to improperly fuse with other TaskPoolStrategy operators. Since the first operator will be implemented with a callable class / actor, we should use ActorPoolStrategy as the compute for the download logical operator.

…56091) ## Why are these changes needed? Update the from uris release test to use the new code from ray-project#55824. ## 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 :( --------- Signed-off-by: Matthew Owen <mowen@anyscale.com> Signed-off-by: sampan <sampan@anyscale.com>

…56091) ## Why are these changes needed? Update the from uris release test to use the new code from ray-project#55824. ## 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 :( --------- Signed-off-by: Matthew Owen <mowen@anyscale.com> Signed-off-by: jugalshah291 <shah.jugal291@gmail.com>

…56091) ## Why are these changes needed? Update the from uris release test to use the new code from ray-project#55824. ## 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 :( --------- Signed-off-by: Matthew Owen <mowen@anyscale.com> Signed-off-by: yenhong.wong <yenhong.wong@grabtaxi.com>

…56091) ## Why are these changes needed? Update the from uris release test to use the new code from ray-project#55824. ## 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 :( --------- Signed-off-by: Matthew Owen <mowen@anyscale.com> Signed-off-by: zac <zac@anyscale.com>

## Why are these changes needed? Update the from uris release test to use the new code from #55824. ## 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 :( --------- Signed-off-by: Matthew Owen <mowen@anyscale.com> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>

…56091) ## Why are these changes needed? Update the from uris release test to use the new code from ray-project#55824. ## 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 :( --------- Signed-off-by: Matthew Owen <mowen@anyscale.com>

adding in updated code to from uris release test

80c790c

Signed-off-by: Matthew Owen <mowen@anyscale.com>

omatthew98 requested a review from bveeramani August 29, 2025 18:59

gemini-code-assist bot reviewed Aug 29, 2025

View reviewed changes

ray-gardener bot added data Ray Data-related issues release-test release test labels Aug 29, 2025

omatthew98 added 2 commits August 29, 2025 12:48

adding in key conversion

0b6e516

Signed-off-by: Matthew Owen <mowen@anyscale.com>

adding in batch_format

0be1690

Signed-off-by: Matthew Owen <mowen@anyscale.com>

iamjustinhsu reviewed Aug 29, 2025

View reviewed changes

adding in supports_fusion=False

7a37bb8

Signed-off-by: Matthew Owen <mowen@anyscale.com>

omatthew98 requested a review from a team as a code owner September 2, 2025 19:17

omatthew98 added 2 commits September 2, 2025 13:14

fix separator

ad96a05

Signed-off-by: Matthew Owen <mowen@anyscale.com>

fix batch construction

48e8a47

Signed-off-by: Matthew Owen <mowen@anyscale.com>

omatthew98 added the go add ONLY when ready to merge, run all tests label Sep 2, 2025

omatthew98 assigned iamjustinhsu Sep 2, 2025

bveeramani approved these changes Sep 3, 2025

View reviewed changes

bveeramani reviewed Sep 3, 2025

View reviewed changes

omatthew98 added 2 commits September 4, 2025 15:08

better fix for fusion bug

d28c7b0

Signed-off-by: Matthew Owen <mowen@anyscale.com>

fix

0e27270

Signed-off-by: Matthew Owen <mowen@anyscale.com>

omatthew98 commented Sep 4, 2025

View reviewed changes

bveeramani merged commit 4a9c49c into ray-project:master Sep 5, 2025
5 checks passed

omatthew98 mentioned this pull request Sep 5, 2025

[data] refactor download expression to use inheritance from AbstractOneToOne #56294

Merged

8 tasks

Conversation

omatthew98 commented Aug 29, 2025

Why are these changes needed?

Related issue number

Checks

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omatthew98 commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants