Add fallback for when CPU count can't be determined

[charlesastaylor]

Summary

psutil.cpu_count(logical=False) returns None when the true count can't be determined. #1212 found this happening on a raspberry pi.

[nonhermitian]

This looks good.

[charlesastaylor]

Should I put something in changelog? Not really sure what warrants going in changelog and what doesn't.

[nonhermitian]

Yeah. Just say you fixed an issue with finding num processes

[charlesastaylor]

@nonhermitian Thanks, done. Did you see my question in the issue thread?

Checking psutil.cpu_count on my pi I noticed that cpu_count(logical=False) returns None but cpu_count(logical=True) returns 4. Is it worth trying value of logical=True when logical=False returns None or just default straight to 1?

[nonhermitian]

In setting logical to False, we are looking for only physical cpus. Things like hyperthreads do not greatly contribute to the parallel power, and increase the memory count. So we try to avoid them. Not sure why the arm processor says no physical cpus.

[mtreinish]

I'm able to reproduce psutil() returning None on one of my arm devices (not a raspberry pi, don't have one handy right now). Looking at the psutil code for getting the physical cpu count: https://github.com/giampaolo/psutil/blob/master/psutil/_pslinux.py#L624-L646 it's expecting a certain format from /proc/cpuinfo (what x86 CPUs have) but the arm format is different, and it doesn't have the 'physical id' key that psutil is searching for. So it fails to parse and returns None. Looking at the code for the logical cpu count they had a similar issue there and fixed it by parsing /proc/stat instead. I'll submit a bug to psutil and work on a fix later.

Other than the merge conflict on the changelog, this LGTM. A unit test would be nice, but it's not worth blocking over for something this simple.

[charlesastaylor]

@mtreinish Yeah I found the same thing when I looked. I came across this closed issue first and stopped at that I'm afraid!

Will fix merge conflict now. Thought about trying to add test but didn't know how I would do it I'm afraid.

[mtreinish]

No worries, fwiw you could do something like:

    @mock.patch('platform.system', return_value='Linux')
    @mock.patch('psutil.virtual_memory')
    @mock.patch('psutil.cpu_count', return_value=None)
    def test_local_hardware_None_cpu_count(self, cpu_count_mock, vmem_mock,
                                           platform_mock):
        result = _util.local_hardware_info()
        self.assertEqual(1, result['cpus'])

In test/python/test_util.py. But it's not worth blocking over, we can always add it later too.

[charlesastaylor]

@mtreinish Thanks! I had no idea about mock so didn't know where to begin for writing a test.

Added test as suggested with some minor adjustments for linting.

[mtreinish]

@charlesastaylor Sigh, well it looks like test_util.py file was removed in #1198 (since it removed the need for the only test that was in there in the patch) so you'll have to just add the file back with only the new test that you're adding here.

[delapuente]

One CPU seems like a good fallback. :p

Thank you.

[diego-plan9]

Thanks @charlesastaylor for the neat fix and for the quick adjustments in this high-activity phase! I'm clicking the approve button and merging, but the real credit for the review is to @nonhermitian, @mtreinish and @delapuente - thanks everyone, and looking forward to more contributions!

Dismissing due to github bureocracy - the changes requested have been performed