Use os.sched_getaffinity to get info about available CPUs #6131
Conversation
|
This pull request introduces 1 alert when merging 30a1837bb5e09b361d2956fb218a71ec3300c101 into 34caa75 - view on LGTM.com new alerts:
|
chia/util/cpus.py
Outdated
| except AttributeError: | ||
| pass | ||
|
|
||
| if os.name == "nt" and cpu_count > 61: |
There was a problem hiding this comment.
Is "nt" generic across all modern Windows versions?
There was a problem hiding this comment.
@hoffmang9 This exact check is in any case being done when trying to compile the chia blockchain, at the point where the code tries to import this external file and this line.
I ran into a problem installing the whole thing, as described in #5158.
There was a problem hiding this comment.
@hoffmang9 TBF I'm not that familiar with python and especially python on windows. I followed some quick online docs, checked existing Chia sources.
And the same check is already used in Chia
(+ the code that Nikolaj mentioned)
According to the docs it should return "nt" for all Windows versions.
Besides WSL or Cygwin envs running on Windows.
There was a problem hiding this comment.
No worries - GH now requires approval for first time contributors to start full CI - hence you got a comment from some serious pain that caused that comment from me in the first place regarding the Python windows issues. My quick review does look like "nt" is safe indeed.
chia/util/cpus.py
Outdated
| pass | ||
|
|
||
| if os.name == "nt" and cpu_count > 61: | ||
| cpu_count = 61 # Windows Server 2016 has an issue https://bugs.python.org/issue26903 |
There was a problem hiding this comment.
Looks you can spare some magic and alternatively do
def get_available_cpus() -> int:
try:
cpu_count = os.sched_getaffinity(0)
except AttributeError:
cpu_count = multiprocessing.cpu_count()
# Note: Windows Server 2016 has an issue https://bugs.python.org/issue26903
if os.name == "nt":
MAX = 61
cpu_count = min(cpu_count, MAX)
return cpu_countThere was a problem hiding this comment.
Good suggestion, I've included it in the change.
pawelchcki
force-pushed
the
better_cpu_count
branch
from
30a1837
to
ec90831
Compare
|
Thank you both for quick review! |
when running inside containers with CPU limits python's multiprocessing.cpu_count() will return total number of CPU's available on the machine. However in reality the process and its children will not be able to be scheduled on all CPUs. This issue is most pronounced with taskset/cpuset is set for the process. Causing scheduling contention over limited number of CPU's. In case of CFS cpu limits, its similar, but due to bursty nature, at least in a short timeframe the processes will be able to be scheduled on all CPUs. There is no straightforward way to read the CPU shares assigned via CFS. So just handling simple case when affinity is explicitly set should be a good start
pawelchcki
force-pushed
the
better_cpu_count
branch
from
ec90831
to
4cc644a
Compare
|
Thanks for your contribution! do you mind fixing this merge conflict? |
|
@mariano54 merge conflict resolved :) |
|
This pull request introduces 1 alert when merging 3abd3f6 into 35dfdbc - view on LGTM.com new alerts:
|
|
This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed. |
github-actions
bot
added
the
stale-pr
|
This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties. |
github-actions
bot
added
the
stale-pr
github-actions
bot
added
the
merge_conflict
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
stale-pr
|
This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties. |
github-actions
bot
added
the
stale-pr
|
Closing in favor of #17409 |
when running inside containers with CPU limits python's
multiprocessing.cpu_count() will return total number of CPU's
available on the machine. However in reality the process and its
children will not be able to be scheduled on all CPUs.
This issue is most pronounced with taskset/cpuset is set for the process.
Causing scheduling contention over limited number of CPU's.
In case of CFS cpu limits, its similar, but due to bursty nature, at least
in a short timeframe the processes will be able to be scheduled on all CPUs.
The overall efficiency however will be reduced still.
There is no straightforward way to read the CPU shares assigned via CFS.
So just handling simple case when affinity is explicitly set should be a good start