-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[data] remove concurrency lock #56798
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] remove concurrency lock #56798
Conversation
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
python/ray/data/_internal/compute.py
Outdated
| max_size: Optional[int] = None, | ||
| initial_size: Optional[int] = None, | ||
| max_tasks_in_flight_per_actor: Optional[int] = None, | ||
| single_threaded: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| single_threaded: bool = True, | |
| enable_true_multi_threading: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add ample commentary to explain the difference
e2ed5f3 to
37b9e50
Compare
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
37b9e50 to
ca15c6d
Compare
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
e3dd37c to
8c97fb7
Compare
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
| f"initial_size={self.initial_size}, " | ||
| f"max_tasks_in_flight_per_actor={self.max_tasks_in_flight_per_actor})" | ||
| f"num_workers={self.num_workers}, " | ||
| f"enable_true_multi_threading={self.enable_true_multi_threading}, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Malformed repr string with misplaced closing parenthesis
The __repr__ method has a closing parenthesis ) at the end of the max_tasks_in_flight_per_actor line, but continues with more fields (num_workers, enable_true_multi_threading, ready_to_total_workers_ratio) followed by another ). This produces malformed output like ActorPoolStrategy(...)num_workers=..., enable_true_multi_threading=..., ...) where fields after the first ) appear outside the constructor notation.
| enable_true_multi_threading: bool = ( | ||
| compute.enable_true_multi_threading | ||
| if isinstance(compute, ActorPoolStrategy) | ||
| else True | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do it inside _get_udf to avoid duplication
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
Signed-off-by: iamjustinhsu <jhsu@anyscale.com>
| if ( | ||
| not is_async_udf | ||
| and isinstance(compute, ActorPoolStrategy) | ||
| and not compute.enable_true_multi_threading | ||
| ): | ||
| udf = make_callable_class_single_threaded(udf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment to explain the behavior here
Signed-off-by: Alexey Kudinkin <alexey.kudinkin@gmail.com>
Why are these changes needed?
Currently, users who specify
max_concurrency>1don't actually experience multi-threaded concurrency in their actors. This PR addresses that by allowing users to override actor poolmax_concurrencybehavior. Changes I mademax_concurrency, they can setenable_true_multi_threading=Truein theirActorComputeStrategyRelated issue number
#55354
Checks
Tests:
NONE YET
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.