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

Prevent pylint.run._cpu_count() from returning 0 #6903

Merged
merged 13 commits into from
Jun 11, 2022
Merged
6 changes: 5 additions & 1 deletion doc/whatsnew/2/2.14/full.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ What's New in Pylint 2.14.2?
----------------------------
Release date: TBA

* Don't crash if ``lint.run._query_cpu()`` is run within a Kubernetes Pod, that has only
a fraction of a cpu core assigned. Just go with one process then.

Closes #6902

* Fixed a false positive in ``consider-using-f-string`` if the left side of a ``%`` is not a string.

Closes #6689
Expand All @@ -17,7 +22,6 @@ Release date: TBA
* Fixed a false positive for ``used-before-assignment`` when a try block returns
but an except handler defines a name via type annotation.


What's New in Pylint 2.14.1?
----------------------------
Release date: 2022-06-06
Expand Down
7 changes: 7 additions & 0 deletions pylint/lint/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ def _query_cpu() -> int | None:
cpu_shares = int(file.read().rstrip())
# For AWS, gives correct value * 1024.
avail_cpu = int(cpu_shares / 1024)

# In K8s Pods also a fraction of a single core could be available
# As multiprocessing is not able to run only a "fraction" of process
# assume we have 1 CPU available
if avail_cpu == 0:
avail_cpu = 1

return avail_cpu
Comment on lines +65 to 68
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if avail_cpu == 0:
avail_cpu = 1
return avail_cpu
return max(avail_cpu, 1)

A little bit more explicit imo. I didn't know 0,1 == 0 so people might forget that as well later on.

Copy link
Contributor Author

@d1gl3 d1gl3 Jun 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0,1 == 0 can't happen here, as it's getting rounded to the next int so avail_cpu is either 0 or >=1 there. Nevertheless, what I also thought of was, if just shortcircuiting the return wouldn't be the more elegant solution:

Suggested change
if avail_cpu == 0:
avail_cpu = 1
return avail_cpu
return avail_cpu or 1

But yours is more explicit though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did apply your suggestion, but this is failing due to avail_cpu being initialized as None (a valid return value for this function imo) which does not work with max. My proposal above also doesn't work here. If the function doesn't find any of the files used to determine cpus, it is just wrong to return 1 cpu available then.

Therefore I just reverse that change to my initial implementaion, only in case of avail_cpu == 0 this should be set automatically to 1



Expand Down
37 changes: 36 additions & 1 deletion tests/test_pylint_runners.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,17 @@
from __future__ import annotations

import os
import pathlib
import sys
from collections.abc import Callable
from unittest.mock import patch
from unittest.mock import MagicMock, mock_open, patch

import pytest
from py._path.local import LocalPath # type: ignore[import]

from pylint import run_epylint, run_pylint, run_pyreverse, run_symilar
from pylint.lint import Run
from pylint.testutils import GenericTestReporter as Reporter


@pytest.mark.parametrize(
Expand All @@ -40,3 +43,35 @@ def test_runner_with_arguments(runner: Callable, tmpdir: LocalPath) -> None:
with pytest.raises(SystemExit) as err:
runner(testargs)
assert err.value.code == 0


def test_pylint_run_jobs_equal_zero_dont_crash_with_cpu_fraction(
tmpdir: LocalPath,
) -> None:
"""Check that the pylint runner does not crash if `pylint.lint.run._query_cpu`
determines only a fraction of a CPU core to be available.
"""
builtin_open = open

def _mock_open(*args, **kwargs):
if args[0] == "/sys/fs/cgroup/cpu/cpu.cfs_quota_us":
return mock_open(read_data=b"-1")(*args, **kwargs)
if args[0] == "/sys/fs/cgroup/cpu/cpu.shares":
return mock_open(read_data=b"2")(*args, **kwargs)
return builtin_open(*args, **kwargs)

pathlib_path = pathlib.Path

def _mock_path(*args, **kwargs):
if args[0] == "/sys/fs/cgroup/cpu/cpu.shares":
return MagicMock(is_file=lambda: True)
return pathlib_path(*args, **kwargs)

filepath = os.path.abspath(__file__)
testargs = [filepath, "--jobs=0"]
with tmpdir.as_cwd():
with pytest.raises(SystemExit) as err:
with patch("builtins.open", _mock_open):
with patch("pylint.lint.run.Path", _mock_path):
Run(testargs, reporter=Reporter())
assert err.value.code == 0