-
Notifications
You must be signed in to change notification settings - Fork 495
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
Use cgroups aware processor count by default #969
Conversation
In containerized environments, a number of CPU cores isn't the same as the available CPUs. In this case, we need to consider cgroups. `concurrent-ruby` now has the method to get that since v1.3.1. I think it's better to use this for setting more container environment friendly default value. Ref: ruby-concurrency/concurrent-ruby#1038
@@ -17,7 +18,7 @@ def determine_number_of_processes(count) | |||
[ | |||
count, | |||
ENV["PARALLEL_TEST_PROCESSORS"], | |||
Parallel.processor_count |
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.
need to remove that method then ?
... best make it call Concurrent.available_processor_count and print a deprecation
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.
Is it OK to remove Parallel.processor_count
? I thought we can't remove it because it consider env(PARALLEL_PROCESSOR_COUNT
), but Concurrent.available_processor_count
not.
Is it better to use Concurrent.available_processor_count
inside Parallel.processor_count
or add a new method like Parallel.available_processor_count
?
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.
it would be awesome to have Parallel.processor_count use Concurrent.available_processor_count
because otherwise the copy-pasting just continues and afaik these methods are supposed to do the same thing
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.
possibly even make Parallel.processor_count
show a deprecation and tell users to use Concurrent.available_processor_count
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.
Thanks. I've created the PR to parallel
.
grosser/parallel#348
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.
solved by using parallel 1.26.0
Changes
In containerized environments, a number of CPU cores isn't the same as the available CPUs. In this case, we need to consider cgroups.
concurrent-ruby
now has the method to get that since v1.3.1. I think it's better to use this for setting more container environment friendly default value.Ref: ruby-concurrency/concurrent-ruby#1038
Thank you for your contribution!
Checklist
master
(if not - rebase it).code introduces user-observable changes.