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

[Core] Make multiprocessing pool raise TypeError when provided a non-iterable in imap or imap_unordered #31799

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

cadedaniel
Copy link
Member

@cadedaniel cadedaniel commented Jan 20, 2023

TL;DR

This PR modifies ray.util.multiprocessing.Pool so that pool.imap and pool.imap_unordered throw TypeError when given non-iterable inputs.

More details

We want the API of ray.util.multiprocessing.Pool to closely resemble that of multiprocessing.Pool. One Ray user pointed out in #24237 that our imap and imap_unordered implementations differ slightly from multiprocessing.Pool in what arguments they accept. See the following usage:

#!/usr/bin/env python3

def main():
    import multiprocessing as mp
    with mp.Pool() as pool:
        assert_throws_on_noniterable(pool.imap)
        assert_throws_on_noniterable(pool.imap_unordered)

    import ray.util.multiprocessing as raymp
    with raymp.Pool() as pool:
        assert_throws_on_noniterable(pool.imap)
        assert_throws_on_noniterable(pool.imap_unordered)

def assert_throws_on_noniterable(method):
    iterable_input = [1, 2, 3]
    noniterable_input = 3

    assert len([_ for _ in method(f, iterable_input)]) == len(iterable_input)

    try:
        [_ for _ in method(f, noniterable_input)]
        assert False, f"expected TypeError for {method}"
    except TypeError:
        pass

def f(_):
    pass

if __name__ == '__main__':
    main()

With this PR, this code produces no exception. Without this PR, this fails:

Traceback (most recent call last):
  File "/data/ray/./cade.py", line 30, in <module>
    main()
  File "/data/ray/./cade.py", line 11, in main
    assert_throws_on_noniterable(pool.imap)
  File "/data/ray/./cade.py", line 22, in assert_throws_on_noniterable
    assert False, f"expected TypeError for {method}"
AssertionError: expected TypeError for <bound method Pool.imap of <ray.util.multiprocessing.pool.Pool object at 0x7f109b9ff640>>

Closes #24237

@cadedaniel cadedaniel added core Issues that should be addressed in Ray Core @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jan 20, 2023
@cadedaniel cadedaniel force-pushed the mp-pool-matches-py-mp-pool branch 4 times, most recently from ac73679 to 67d71e9 Compare January 20, 2023 23:23
@cadedaniel cadedaniel changed the title [Draft] Make multiprocessing pool raise TypeError when provided a non-iterable in map [Core] Make multiprocessing pool raise TypeError when provided a non-iterable in imap or imap_unordered Jan 20, 2023
@cadedaniel cadedaniel marked this pull request as ready for review January 20, 2023 23:24
@cadedaniel cadedaniel added do-not-merge Do not merge this PR! Ray 2.4 and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jan 20, 2023
@cadedaniel
Copy link
Member Author

We will wait until Ray 2.4 to merge this. In Ray 2.3 we will raise a deprecation warning, see PR #31845

@cadedaniel cadedaniel changed the title [Core] Make multiprocessing pool raise TypeError when provided a non-iterable in imap or imap_unordered [After 2.3 branch cut] [Core] Make multiprocessing pool raise TypeError when provided a non-iterable in imap or imap_unordered Jan 22, 2023
rkooo567 pushed a commit that referenced this pull request Jan 23, 2023
…nd imap_unordered methods of ray.util.multiprocessing.Pool (#31845)

Context: #24237

We will raise this warning in Ray 2.3. For Ray 2.4, we will merge #31799 which causes a TypeError to be raised instead.
Comment on lines 517 to 532
with pytest.raises(TypeError, match="object is not iterable"):
pool.imap(fn, non_iterable)

with pytest.raises(TypeError, match="object is not iterable"):
pool.imap_unordered(fn, non_iterable)

with pytest.raises(TypeError, match="object is not iterable"):
pool.map(fn, non_iterable)

with pytest.raises(TypeError, match="object is not iterable"):
pool.map_async(fn, non_iterable)

with pytest.raises(TypeError, match="must be an iterable, not"):
pool.starmap(fn, [non_iterable])

with pytest.raises(TypeError, match="must be an iterable, not"):
Copy link
Contributor

Choose a reason for hiding this comment

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

where are these error messages coming from, the iter call?

Not sure what it looks like, but should we instead wrap it and write a more user-friendly error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can; I opted not to because this is the same error that Pool from Python multiprocessing raises.

@edoakes edoakes added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 23, 2023
@cadedaniel cadedaniel assigned cadedaniel and unassigned scv119, edoakes and rkooo567 Feb 22, 2023
…or on non-iterable arguments, matching Python multiprocessing.Pool behavior.

Signed-off-by: Cade Daniel <[email protected]>
@cadedaniel cadedaniel changed the title [After 2.3 branch cut] [Core] Make multiprocessing pool raise TypeError when provided a non-iterable in imap or imap_unordered [Core] Make multiprocessing pool raise TypeError when provided a non-iterable in imap or imap_unordered Mar 7, 2023
@cadedaniel cadedaniel removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. do-not-merge Do not merge this PR! labels Mar 7, 2023
@cadedaniel cadedaniel assigned scv119 and jjyao and unassigned cadedaniel Mar 8, 2023
@cadedaniel
Copy link
Member Author

review or merge pls cc @scv119 @jjyao

@scv119
Copy link
Contributor

scv119 commented Mar 8, 2023

nice!

@scv119 scv119 merged commit acd5296 into ray-project:master Mar 8, 2023
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request Mar 21, 2023
…iterable in imap or imap_unordered ray-project#31799

This PR modifies ray.util.multiprocessing.Pool so that pool.imap and pool.imap_unordered throw TypeError when given non-iterable inputs.
Signed-off-by: Cade Daniel <[email protected]>
Signed-off-by: Jack He <[email protected]>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
…iterable in imap or imap_unordered ray-project#31799

This PR modifies ray.util.multiprocessing.Pool so that pool.imap and pool.imap_unordered throw TypeError when given non-iterable inputs.
Signed-off-by: Cade Daniel <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
…iterable in imap or imap_unordered ray-project#31799

This PR modifies ray.util.multiprocessing.Pool so that pool.imap and pool.imap_unordered throw TypeError when given non-iterable inputs.
Signed-off-by: Cade Daniel <[email protected]>
@cadedaniel cadedaniel deleted the mp-pool-matches-py-mp-pool branch April 5, 2023 18:09
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
…iterable in imap or imap_unordered ray-project#31799

This PR modifies ray.util.multiprocessing.Pool so that pool.imap and pool.imap_unordered throw TypeError when given non-iterable inputs.
Signed-off-by: Cade Daniel <[email protected]>
Signed-off-by: elliottower <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
…iterable in imap or imap_unordered ray-project#31799

This PR modifies ray.util.multiprocessing.Pool so that pool.imap and pool.imap_unordered throw TypeError when given non-iterable inputs.
Signed-off-by: Cade Daniel <[email protected]>
Signed-off-by: Jack He <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues that should be addressed in Ray Core Ray 2.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core][multiprocessing] Make Pool.imap() and Pool.imap_unordered() throw TypeError on non-iterable/iterator
5 participants