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.

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

[data] [docs] Generalize fix for converting lists to np.ndarray in UDFs #34930

Merged
merged 2 commits into from
May 2, 2023

Conversation

ericl
Copy link
Contributor

@ericl ericl commented May 1, 2023

Why are these changes needed?

The fix from https://github.com/ray-project/ray/pull/34734/files is at slightly the wrong point. Move it into the batch builder so that it also applies to other API sites such as map_groups.

Signed-off-by: Eric Liang <[email protected]>
@pcmoritz
Copy link
Contributor

pcmoritz commented May 2, 2023

Should the list part also be removed in line 55 of python/ray/data/_internal/planner/map_batches.py (since it presumably already got converted?)

                    if not isinstance(value, (np.ndarray, list)):

I don't know this code super well so ^ might be wrong :)

Signed-off-by: Eric Liang <[email protected]>
@ericl
Copy link
Contributor Author

ericl commented May 2, 2023

Should the list part also be removed in line 55 of python/ray/data/_internal/planner/map_batches.py (since it presumably already got converted?)

This check is occurring earlier in the process, so we still need to be permissive on what types are allowed.

@ericl ericl added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label May 2, 2023
@ericl ericl merged commit fef04ac into ray-project:master May 2, 2023
architkulkarni pushed a commit to architkulkarni/ray that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants