ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/promptflow
File: src/promptflow-azure/promptflow/azure/_utils/_token_cache.py (Python)

Code before:
import time

from promptflow.core._connection_provider._utils import get_arm_token


class SingletonMeta(type):
    _instances = {}

    def __call__(cls, *args, **kwargs):
        if cls not in cls._instances:
            instance = super().__call__(*args, **kwargs)
            cls._instances[cls] = instance
        return cls._instances[cls]


class ArmTokenCache(metaclass=SingletonMeta):
    DEFAULT_TTL_SECS = 1800

    def __init__(self):
        self._ttl_secs = self.DEFAULT_TTL_SECS
        self._cache = {}

    def _is_token_valid(self, entry):
        return time.time() < entry["expires_at"]

    def get_token(self, credential):
        if credential in self._cache:
            entry = self._cache[credential]
            if self._is_token_valid(entry):
                return entry["token"]

        token = self._fetch_token(credential)
        self._cache[credential] = {"token": token, "expires_at": time.time() + self._ttl_secs}
        return token

    def _fetch_token(self, credential):
        return get_arm_token(credential=credential)


Review comment (performance):
should we do refresh for token which is about to expire in near time?

azure-identity will do it for 300 seconds
https://github.com/Azure/azure-sdk-for-python/blob/9fb4c52d988ceb3b9583b2dff48d2e0304228590/sdk/identity/azure-identity/azure/identity/_internal/shared_token_cache.py#L240

Code after:
# ---------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# ---------------------------------------------------------
import time

import jwt

from promptflow.core._connection_provider._utils import get_arm_token


class SingletonMeta(type):
    _instances = {}

    def __call__(cls, *args, **kwargs):
        if cls not in cls._instances:
            instance = super().__call__(*args, **kwargs)
            cls._instances[cls] = instance
        return cls._instances[cls]


class ArmTokenCache(metaclass=SingletonMeta):
    TOKEN_REFRESH_THRESHOLD_SECS = 300

    def __init__(self):
        self._cache = {}

    def _is_token_valid(self, entry):
        current_time = time.time()
        return (entry["expires_at"] - current_time) >= self.TOKEN_REFRESH_THRESHOLD_SECS

    def get_token(self, credential):
        if credential in self._cache:
            entry = self._cache[credential]
            if self._is_token_valid(entry):
                return entry["token"]

        token = self._fetch_token(credential)
        decoded_token = jwt.decode(token, options={"verify_signature": False, "verify_aud": False})
        expiration_time = decoded_token.get("exp", time.time())
        self._cache[credential] = {"token": token, "expires_at": expiration_time}
        return token

    def _fetch_token(self, credential):
        return get_arm_token(credential=credential)
Repository: microsoft/qlib
File: qlib/data/cache.py (Python)

Code before:
    get_redis_connection,
    read_bin,
    parse_field,
    remove_fields_space,
    normalize_cache_fields,
    normalize_cache_instruments,
)

from ..log import get_module_logger
from .base import Feature

from .ops import *


class QlibCacheException(RuntimeError):
    pass


class MemCacheUnit(abc.ABC):
    """Memory Cache Unit."""

    def __init__(self, *args, **kwargs):
        self.size_limit = kwargs.pop("size_limit", 0)
        self.limit_type = kwargs.pop("limit_type", "length")

        assert self.limit_type in ["length", "sizeof"], ValueError("limit_type shoule be one of ['length', 'sizeof']")
        assert self.size_limit >= 0, ValueError("size_limit shoule not be negative.The default 0 means unlimited!")

        # limit_flag: whether to popitem or not
        self._limit_flag = 1 if self.size_limit > 0 else 0
        self._size = 0
        self.od = OrderedDict()

    def __setitem__(self, key, value):
        # TODO: thread safe?__setitem__ failure might cause inconsistent size?

        # precalculate the size after od.__setitem__
        self._adjust_size(key, value)

        self.od.__setitem__(key, value)

        # move the key to end,make it latest
        self.od.move_to_end(key)

        # pop the oldest items beyond size limit
        while self._size * self._limit_flag > self.size_limit:
            self.popitem(last=False)

    def __getitem__(self, key):
        v = self.od.__getitem__(key)
        self.od.move_to_end(key)

Review comment (suggestion):
If  we  have two subclasses, `limit_type` is not requried anymore.

Code after:
    get_redis_connection,
    read_bin,
    parse_field,
    remove_fields_space,
    normalize_cache_fields,
    normalize_cache_instruments,
)

from ..log import get_module_logger
from .base import Feature

from .ops import *


class QlibCacheException(RuntimeError):
    pass


class MemCacheUnit(abc.ABC):
    """Memory Cache Unit."""

    def __init__(self, *args, **kwargs):
        self.size_limit = kwargs.pop("size_limit", 0)
        self._size = 0
        self.od = OrderedDict()

    def __setitem__(self, key, value):
        # TODO: thread safe?__setitem__ failure might cause inconsistent size?

        # precalculate the size after od.__setitem__
        self._adjust_size(key, value)

        self.od.__setitem__(key, value)

        # move the key to end,make it latest
        self.od.move_to_end(key)

        if self.limited:
            # pop the oldest items beyond size limit
            while self._size > self.size_limit:
                self.popitem(last=False)

    def __getitem__(self, key):
        v = self.od.__getitem__(key)
        self.od.move_to_end(key)
        return v

    def __contains__(self, key):
        return key in self.od

    def __len__(self):
Repository: microsoft/qlib
File: qlib/data/cache.py (Python)

Code before:
    normalize_cache_fields,
    normalize_cache_instruments,
)

from ..log import get_module_logger
from .base import Feature

from .ops import *


class QlibCacheException(RuntimeError):
    pass


class MemCacheUnit(abc.ABC):
    """Memory Cache Unit."""

    def __init__(self, *args, **kwargs):
        self.size_limit = kwargs.pop("size_limit", 0)
        self.limit_type = kwargs.pop("limit_type", "length")

        assert self.limit_type in ["length", "sizeof"], ValueError("limit_type shoule be one of ['length', 'sizeof']")
        assert self.size_limit >= 0, ValueError("size_limit shoule not be negative.The default 0 means unlimited!")

        # limit_flag: whether to popitem or not
        self._limit_flag = 1 if self.size_limit > 0 else 0
        self._size = 0
        self.od = OrderedDict()

    def __setitem__(self, key, value):
        # TODO: thread safe?__setitem__ failure might cause inconsistent size?

        # precalculate the size after od.__setitem__
        self._adjust_size(key, value)

        self.od.__setitem__(key, value)

        # move the key to end,make it latest
        self.od.move_to_end(key)

        # pop the oldest items beyond size limit
        while self._size * self._limit_flag > self.size_limit:
            self.popitem(last=False)

    def __getitem__(self, key):
        v = self.od.__getitem__(key)
        self.od.move_to_end(key)
        return v

    def __contains__(self, key):
        return key in self.od

Review comment (suggestion):
I think merge `_limit_flag` into variable `size_limit` will make it simpler
For exmaple,   `size_limit == -1`   indicates no limitation

Code after:
    normalize_cache_fields,
    normalize_cache_instruments,
)

from ..log import get_module_logger
from .base import Feature

from .ops import *


class QlibCacheException(RuntimeError):
    pass


class MemCacheUnit(abc.ABC):
    """Memory Cache Unit."""

    def __init__(self, *args, **kwargs):
        self.size_limit = kwargs.pop("size_limit", 0)
        self._size = 0
        self.od = OrderedDict()

    def __setitem__(self, key, value):
        # TODO: thread safe?__setitem__ failure might cause inconsistent size?

        # precalculate the size after od.__setitem__
        self._adjust_size(key, value)

        self.od.__setitem__(key, value)

        # move the key to end,make it latest
        self.od.move_to_end(key)

        if self.limited:
            # pop the oldest items beyond size limit
            while self._size > self.size_limit:
                self.popitem(last=False)

    def __getitem__(self, key):
        v = self.od.__getitem__(key)
        self.od.move_to_end(key)
        return v

    def __contains__(self, key):
        return key in self.od

    def __len__(self):
        return self.od.__len__()

    def __repr__(self):
        return f"{self.__class__.__name__}<size_limit:{self.size_limit if self.limited else 'no limit'} total_size:{self._size}>\n{self.od.__repr__()}"
Repository: microsoft/qlib
File: qlib/data/cache.py (Python)

Code before:
    def __len__(self):
        return self.od.__len__()

    def __repr__(self):
        return f"{self.__class__.__name__}<limit_type:{self.limit_type} size_limit:{self.size_limit if self.is_limited else 'no limit'} total_size:{self._size}>\n{self.od.__repr__()}"

    def set_limit_size(self, limit):
        self.size_limit = limit

    def set_limit(self, is_limited=True):
        self._limit_flag = 1 if is_limited else 0

    @property
    def is_limited(self):
        return bool(self._limit_flag)

    @property
    def total_size(self):
        return self._size

    def clear(self):
        self._size = 0
        self.od.clear()

    @abc.abstractmethod
    def _adjust_size(self, key, value):
        raise NotImplementedError

    @abc.abstractmethod
    def popitem(self, last=True):
        raise NotImplementedError

    @abc.abstractmethod
    def pop(self, key):
        raise NotImplementedError


class MemCacheLengthUnit(MemCacheUnit):
    def __init__(self, size_limit=0):
        super().__init__(size_limit=size_limit, limit_type="length")

    def _adjust_size(self, key, value):
        if key not in self.od:
            self._size += 1

    def popitem(self, last=True):
        k, v = self.od.popitem(last=last)
        self._size -= 1

        return k, v


Review comment (suggestion):
I think all the differences between the subclasses are the way to calculate the size of value.
Other parts  can be shared.
I think we could make  only one method to calculate size of value overridable. Other logics can be shared in the base class (e.g.  pop, popitem, adjust_size ... ).

Code after:
    def set_limit_size(self, limit):
        self.size_limit = limit

    @property
    def limited(self):
        """whether memory cache is limited"""
        return self.size_limit > 0

    @property
    def total_size(self):
        return self._size

    def clear(self):
        self._size = 0
        self.od.clear()

    def popitem(self, last=True):
        k, v = self.od.popitem(last=last)
        self._size -= self._get_value_size(v)

        return k, v

    def pop(self, key):
        v = self.od.pop(key)
        self._size -= self._get_value_size(v)

        return v

    def _adjust_size(self, key, value):
        if key in self.od:
            self._size -= self._get_value_size(self.od[key])

        self._size += self._get_value_size(value)

    @abc.abstractmethod
    def _get_value_size(self, value):
        raise NotImplementedError


class MemCacheLengthUnit(MemCacheUnit):
    def __init__(self, size_limit=0):
        super().__init__(size_limit=size_limit)

    def _get_value_size(self, value):
        return 1


class MemCacheSizeofUnit(MemCacheUnit):
    def __init__(self, size_limit=0):
        super().__init__(size_limit=size_limit)
Repository: microsoft/qlib
File: qlib/contrib/model/pytorch_utils.py (Python)

Code before:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

import numpy as np
import torch.nn as nn


def count_parameters(model_or_parameters, unit="mb"):
    if isinstance(model_or_parameters, nn.Module):
        counts = np.sum(np.prod(v.size()) for v in model_or_parameters.parameters())
    else:
        counts = np.sum(np.prod(v.size()) for v in model_or_parameters)
    if unit.lower() == "mb":
        counts /= 1e6
    elif unit.lower() == "kb":
        counts /= 1e3
    elif unit.lower() == "gb":
        counts /= 1e9
    elif unit is not None:
        raise ValueError("Unknow unit: {:}".format(unit))
    return counts


Review comment (bug):
From my perspective, it is not necessary to make this feature as a single function, which actually can be implemented in one line. Here are some [resources](https://pytorch.org/docs/stable/generated/torch.numel.html) and references ([link1](https://discuss.pytorch.org/t/how-do-i-check-the-number-of-parameters-of-a-model/4325/25), [link2](https://stackoverflow.com/questions/49201236/check-the-total-number-of-parameters-in-a-pytorch-model)) may be useful.

If you really want to make it a decent feature, I suggest you can print the number of parameters in a nicer way (for example, similar to `Keras`'s stdout). But this should be carefully design, otherwise it will make the output very messy.

Code after:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

import torch.nn as nn


def count_parameters(models_or_parameters, unit="m"):
    """
    This function is to obtain the storage size unit of a (or multiple) models.

    Parameters
    ----------
    models_or_parameters : PyTorch model(s) or a list of parameters.
    unit : the storage size unit.

    Returns
    -------
    The number of parameters of the given model(s) or parameters.
    """
    if isinstance(models_or_parameters, nn.Module):
        counts = sum(v.numel() for v in models_or_parameters.parameters())
    elif isinstance(models_or_parameters, nn.Parameter):
        counts = models_or_parameters.numel()
    elif isinstance(models_or_parameters, (list, tuple)):
        return sum(count_parameters(x, unit) for x in models_or_parameters)
    else:
        counts = sum(v.numel() for v in models_or_parameters)
    unit = unit.lower()
    if unit == "kb" or unit == "k":
        counts /= 2 ** 10
    elif unit == "mb" or unit == "m":
        counts /= 2 ** 20
    elif unit == "gb" or unit == "g":
Repository: microsoft/qlib
File: qlib/log.py (Python)

Code before:
        ----------
        name :
            name
        show_start :
            show_start
        """
        if show_start:
            cls.timer_logger.info(f"{name} Begin")
        cls.set_time_mark()
        try:
            yield None
        finally:
            pass
        cls.log_cost_time(info=f"{name} Done")


def set_log_with_config(log_config: Dict[Text, Any]):
    """set log with config

    :param log_config:
    :return:
    """
    logging_config.dictConfig(log_config)


def set_log_basic_config(filename: Optional[Text] = None, format: Optional[Text] = None, level: Optional[int] = None):
    """
    Set the basic configuration for the logging system.
    See details at https://docs.python.org/3/library/logging.html#logging.basicConfig

    :param filename: str or None
        The path to save the logs.
    :param format: the logging format
    :param level: int
    :return: Logger
        Logger object.
    """
    if level is None:
        level = C.logging_level

    if format is None:
        format = C.logging_config["formatters"]["logger_format"]["format"]

    logging.basicConfig(filename=filename, format=format, level=level)


class LogFilter(logging.Filter):
    def __init__(self, param=None):
        self.param = param

    @staticmethod

Review comment (suggestion):
Most of the loggers used in `Qlib` are initialized through the function `get_module_logger`. If you want to do some enhancement about setting the `logger`, you may want to make some modifications upon that function instead of creating a new one. This function seems to be a little bit redundant.

Code after:
        ----------
        name :
            name
        show_start :
            show_start
        """
        if show_start:
            cls.timer_logger.info(f"{name} Begin")
        cls.set_time_mark()
        try:
            yield None
        finally:
            pass
        cls.log_cost_time(info=f"{name} Done")


def set_log_with_config(log_config: Dict[Text, Any]):
    """set log with config

    :param log_config:
    :return:
    """
    logging_config.dictConfig(log_config)


class LogFilter(logging.Filter):
    def __init__(self, param=None):
        self.param = param

    @staticmethod
    def match_msg(filter_str, msg):
        match = False
        try:
            if re.match(filter_str, msg):
                match = True
        except Exception:
            pass
        return match

    def filter(self, record):
        allow = True
        if isinstance(self.param, str):
            allow = not self.match_msg(self.param, record.msg)
        elif isinstance(self.param, list):
            allow = not any([self.match_msg(p, record.msg) for p in self.param])
        return allow
Repository: microsoft/qlib
File: qlib/log.py (Python)

Code before:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.


import logging
from typing import Optional, Text, Dict, Any
import re
from logging import config as logging_config
from time import time
from contextlib import contextmanager

from .config import C


class MetaLogger(type):
    def __new__(cls, name, bases, dict):
        wrapper_dict = logging.Logger.__dict__.copy()
        wrapper_dict.update(dict)
        wrapper_dict["__doc__"] = logging.Logger.__doc__
        del wrapper_dict["__reduce__"]  # make Logger object can be pickled
        return type.__new__(cls, name, bases, wrapper_dict)


class QlibLogger(metaclass=MetaLogger):
    """
    Customized logger for Qlib.
    """

    def __init__(self, module_name):
        self.module_name = module_name
        self.level = 0

    def __getstate__(self):
        return vars(self)

    def __setstate__(self, state):
        vars(self).update(state)

    def __reduce__(self):
        return (QlibLogger, (self.module_name,))

    @property
    def logger(self):
        logger = logging.getLogger(self.module_name)
        logger.setLevel(self.level)

Review comment (suggestion):
I'm little confused about the current implementation.

If the problem is that copying the pickle related magic methods will result in error when pickling, why don't we just exclude them when copying methods dict.

![image](https://user-images.githubusercontent.com/465606/116550644-97c5ad80-a929-11eb-92ff-87cfcd2723f2.png)


Code after:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.


import logging
from typing import Optional, Text, Dict, Any
import re
from logging import config as logging_config
from time import time
from contextlib import contextmanager

from .config import C


class MetaLogger(type):
    def __new__(cls, name, bases, dict):
        wrapper_dict = logging.Logger.__dict__.copy()
        for key in wrapper_dict:
            if key not in dict and key != "__reduce__":
                dict[key] = wrapper_dict[key]
        return type.__new__(cls, name, bases, dict)


class QlibLogger(metaclass=MetaLogger):
    """
    Customized logger for Qlib.
    """

    def __init__(self, module_name):
        self.module_name = module_name
        self.level = 0

    @property
    def logger(self):
        logger = logging.getLogger(self.module_name)
        logger.setLevel(self.level)
        return logger

    def setLevel(self, level):
        self.level = level

    def __getattr__(self, name):
        # During unpickling, python will call __getattr__. Use this line to avoid maximum recursion error.
        if name in {"__setstate__"}:
            raise AttributeError
Repository: microsoft/qlib
File: qlib/log.py (Python)

Code before:


class LogFilter(logging.Filter):
    def __init__(self, param=None):
        self.param = param

    @staticmethod
    def match_msg(filter_str, msg):
        match = False
        try:
            if re.match(filter_str, msg):
                match = True
        except Exception:
            pass
        return match

    def filter(self, record):
        allow = True
        if isinstance(self.param, str):
            allow = not self.match_msg(self.param, record.msg)
        elif isinstance(self.param, list):
            allow = not any([self.match_msg(p, record.msg) for p in self.param])
        return allow


@contextmanager
def set_global_logger_level(level: int):
    _handler_level_map = {}
    qlib_logger = logging.root.manager.loggerDict.get("qlib", None)
    if qlib_logger is not None:
        for _handler in qlib_logger.handlers:
            _handler_level_map[_handler] = _handler.level
            _handler.level = level
    try:
        yield
    finally:
        for _handler, _level in _handler_level_map.items():
            _handler.level = _level


Review comment (suggestion):
@zhupr   Please keep the previous  `set_global_logger_level` function and create a new `contextmanager`.
I think they are both very useful.
Besides, please add detailed docstrings about the feature. 

Thanks



Code after:


class LogFilter(logging.Filter):
    def __init__(self, param=None):
        self.param = param

    @staticmethod
    def match_msg(filter_str, msg):
        match = False
        try:
            if re.match(filter_str, msg):
                match = True
        except Exception:
            pass
        return match

    def filter(self, record):
        allow = True
        if isinstance(self.param, str):
            allow = not self.match_msg(self.param, record.msg)
        elif isinstance(self.param, list):
            allow = not any([self.match_msg(p, record.msg) for p in self.param])
        return allow


def set_global_logger_level(level: int, return_orig_handler_level: bool = False):
    """set qlib.xxx logger handlers level

    Parameters
    ----------
    level: int
        logger level

    return_orig_handler_level: bool
        return origin handler level map

    Examples
    ---------

        .. code-block:: python

            import qlib
            import logging
            from qlib.log import get_module_logger, set_global_logger_level
            qlib.init()

            tmp_logger_01 = get_module_logger("tmp_logger_01", level=logging.INFO)
            tmp_logger_01.info("1. tmp_logger_01 info show")

            global_level = logging.WARNING + 1
            set_global_logger_level(global_level)