Fix concurrent actor starting too many threads.#14927
Fix concurrent actor starting too many threads.#14927raulchen merged 12 commits intoray-project:masterfrom
Conversation
edoakes
left a comment
There was a problem hiding this comment.
LGTM but see comments on tests
| RAY_LOG(INFO) << "Setting actor as async with max_concurrency=" << max_concurrency | ||
| << ", creating new fiber thread."; | ||
| pool_.reset(new BoundedExecutor(max_concurrency)); |
There was a problem hiding this comment.
should we assert here that the pool is only ever created once?
python/ray/tests/test_basic_2.py
Outdated
| assert r1 == r2 == r3 | ||
|
|
||
|
|
||
| def test_actor_max_concurrency(ray_start_regular): |
There was a problem hiding this comment.
Can we also test for the number of threads actually started? You could make a bunch of calls that return a thread ID (using https://docs.python.org/3/library/threading.html#threading.current_thread) and assert the set is the expected length. We should also do it for multiple callers (to catch the bug you identified).
python/ray/tests/test_basic_2.py
Outdated
| self.concurrent_tasks += 1 | ||
| self.max_concurrency = max(self.max_concurrency, | ||
| self.concurrent_tasks) | ||
| time.sleep(0.5) |
There was a problem hiding this comment.
let's avoid sleeping here please. you can use the ray.test_utils.SignalActor to control when to block/unblock explicitly
|
@edoakes thanks, all comments are addressed |
edoakes
left a comment
There was a problem hiding this comment.
@raulchen looks good, but tests are failing:
https://travis-ci.com/github/ray-project/ray/jobs/494776823#L1076
This reverts commit 3e1a043.
Why are these changes needed?
Fixes #14894
Related issue number
Checks
scripts/format.shto lint the changes in this PR.