MyGodItsFull0fStars commented 1 month ago

Proposed Refactoring Changes

This pull request contains proposed changes to cpu.py.

Docstrings

I added some docstrings to methods and classes that are publicly visible.

CPU Details

In IntelRAPL, the variable self._cpu_details was initialised, but never used later on, as a new variant without the leading _ called self.cpu_details was introduced. As public functions are available, the self.cpu_details mutations were changed to self._cpu_details.

Pylint

Refactored multiple sections with help of Pylint.

W0201 - attribute-defined-outside-init

Pylint source

Added self._windows_exec_backup = None to __init__() of Intel Power Gadget (line 93), as it was defined within a function.

W0707 - raise-missing-from

Pylint source

Changed code in:

except PermissionError as e:
    raise PermissionError(
        "Unable to read Intel RAPL files for CPU power, we will use a constant for your CPU power."
        + " Please view https://github.com/mlco2/codecarbon/issues/244"
        + f" for workarounds : {e}"
    )
        + " for workarounds : %s",
        e,
    ) from e

Note, it might make sense to raise another error type instead of raising the same one again, as the from e is usually used to chain exceptions and provide a traceback.

R1734 - use-list-literal

Pylint source

Replaced list() with [] as this approach is faster, since the additional function call is omitted.

R1735 - use-dict-literal

Pylint source

Replaced dict() with {} as this approach is faster, since the additional function call is omitted.

W1201 - logging-not-lazy

Pylint source

Refactored logging functions to now use lazy string generation.

vercel[bot] commented 1 month ago

@MyGodItsFull0fStars is attempting to deploy a commit to the Code Carbon Team on Vercel.

A member of the Team first needs to authorize it.

benoit-cty commented 4 weeks ago

Thanks ! I would prefer to keep f-string as I find them more readable, and because of PEP-0498. Can you add the option logging-format-style=fstr to PyInt ?

EDIT: Well it seems that % formatting is more robust and should be preferred : https://github.com/pylint-dev/pylint/issues/2395#issuecomment-822546395

MyGodItsFull0fStars commented 3 weeks ago

Thanks for accepting the PR!

To your previous comment, I also prefer the readability of f-strings but yes, this variant is (sadly) more robust.

mlco2 / codecarbon