Skip to content
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

[python-package] fix mypy error about cpu_count() methods #5786

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

jameslamb
Copy link
Collaborator

Contributes to #3756.
Contributes to #3867.

Fixes the following mypy errors.

python-package/lightgbm/sklearn.py:696: error: Incompatible return value type (got "Union[int, Any, None]", expected "int")  [return-value]

lightgbm tries a few different approaches to determine the number of phyiscal/logical CPUs based on conditional imports. Adding type hints explicitly telling mypy that those methods will return integers resolves this issue.

In this case, mypy caught a legitimate possible source of bugs 🎉

If joblib.cpu_count can't be imported but psutil.cpu_count can, psutil.cpu_count() is used to determine the number of available CPUs.

According to the docs at https://psutil.readthedocs.io/en/latest/#psutil.cpu_count, that function returns None if it can't determine the number of CPUs. That's problematic because the result of that function is used unconditionally as if it was an integer:

n_jobs = max(_LGBMCpuCount(only_physical_cores=False) + 1 + n_jobs, 1)

None + 1 - 1
# TypeError: unsupported operand type(s) for +: 'NoneType' and 'int'

To deal with that, this PR proposes falling back to a value of 1 if psutil.cpu_count() returns None. That might mean that the user's physical resources are underutilized, but that's better than a runtime error.

The other two methods can't return None?

Right.

joblib.cpu_count() calls loky.cpu_count() (code link). I'm not 100% sure, but looks to me like like loky.cpu_count() cannot return None based on the code paths at https://github.com/joblib/loky/blob/047d80623b7cf2d43500ed56ee0320b3e41c3f81/loky/backend/context.py#L77.

multiprocessing.cpu_count() raises a NotImplementedError if it can't figure out the number of CPUs: https://docs.python.org/3/library/multiprocessing.html#multiprocessing.cpu_count.

@jameslamb jameslamb merged commit a6bf4ad into master Mar 16, 2023
@jameslamb jameslamb deleted the ci/mypy-misc branch March 16, 2023 14:02
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed.
To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues
including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants