laurent-laporte-pro / deprecated

Python @deprecated decorator to deprecate old python classes, functions or methods.
MIT License
307 stars 34 forks source link

[ENH] Stacklevel offset #68

Closed coroa closed 1 year ago

coroa commented 1 year ago

Over at https://github.com/coroa/pandas-indexing/pull/27, I am about to deprecate a pandas accessor, but since these accessors are classes, which are instantiated by pandas upon access the warning is not emitted since the stacklevel is too low.

MWE

import pandas as pd
from deprecated import deprecated

@pd.api.extensions.register_dataframe_accessor("idx")
@deprecated
class IdxAccessor:
    def __init__(self, pandas_obj):
        self._obj = pandas_obj

df = pd.DataFrame()
df.idx

will only emit with a ~warnings.simplefilter("always")~ warnings.simplefilter("default") (edit: changed to include "default" which was pointed out below), since the callstack looks like:

  Cell In[4], line 11
    df.idx
  File ~/.local/conda/envs/aneris2/lib/python3.10/site-packages/pandas/core/accessor.py:182 in __get__
    accessor_obj = self._accessor(obj)
  File ~/.local/conda/envs/aneris2/lib/python3.10/site-packages/deprecated/classic.py:169 in wrapped_cls
    warnings.warn(msg, category=self.category, stacklevel=_class_stacklevel)

so that the last stacklevel=2 setting points to the pandas.core.accessor module instead of the user code.

Proposal

Would you accept a PR to add a stacklevel_offset or stacklevel argument to deprecated which would either be added like:

    warnings.warn(msg, category=self.category, stacklevel=_class_stacklevel + stacklevel_offset)

or could be used to replace the default stacklevel, like:

    warnings.warn(msg, category=self.category, stacklevel=_class_stacklevel if stacklevel is None else stacklevel)
tantale commented 1 year ago

ANALYSIS

The issue with the current code is that the warning message is not being displayed due to the deprecation warning being triggered from the pandas.core.accessor module, which is not the __main__ module. According to the documentation, all DeprecationWarning warnings are ignored unless they are triggered by code in the __main__ module.

To understand the current warning filter settings, you can display them using the following code:

import warnings
import pprint

pprint.pprint(warnings.filters, width=200)

This will provide the output:

[('default', None, <class 'DeprecationWarning'>, '__main__', 0),
 ('ignore', None, <class 'DeprecationWarning' >, None, 0),
 ('ignore', None, <class 'PendingDeprecationWarning' >, None, 0),
 ('ignore', None, <class 'ImportWarning' >, None, 0),
 ('ignore', None, <class 'ResourceWarning' >, None, 0)]

To address the issue in your demo script, you can add a filter:

import warnings

import pandas as pd
from deprecated import deprecated

@pd.api.extensions.register_dataframe_accessor("idx")
@deprecated()
class IdxAccessor:
    def __init__(self, pandas_obj):
        self._obj = pandas_obj

# Always trigger deprecation warnings
warnings.filterwarnings("default", category=DeprecationWarning)

df = pd.DataFrame()
_ = df.idx

This will result in the following warning message:

/path/to/venv/lib/python3.8/site-packages/pandas/core/accessor.py:224: DeprecationWarning: Call to deprecated class IdxAccessor.
  accessor_obj = self._accessor(obj)

If I understand your issue correctly, you want to have a warning message when the user accesses the idx attribute of the df object. Additionally, you would like an error message that indicates the file and line where this call is made in the code. Hence, your question regarding the stacklevel.

Here, df.idx is implemented using a descriptor (a kind of property) that constructs an instance of the IdxAccessor class upon first usage (caching mechanism).

With the current implementation of the @deprecated decorator, the triggering of the warning message actually occurs at the source code level of the descriptor and not at the user's code level.

This implies two things:

  1. The stacklevel needs to be adjusted to display the location where the descriptor is called, rather than where it is implemented (in the pandas.core.accessor.CachedAccessor class). In your case, increasing the stacklevel by 1 should suffice.

  2. The filtering of warnings using the action parameter of the warnings.filterwarnings function occurs at the point of initializing the IdxAccessor object, which is where the descriptor is implemented. However, for the calculation of the source code position (module name and line number), the stacklevel is taken into account. In other words, for the filtering to work correctly when using action="default" or action="module", the stacklevel also needs to be adjusted to point to the user's code position. The code below illustrates the relationship between the action parameter of filtering and the stacklevel:

    import warnings
    
    def foo():
       warnings.warn("warn here", stacklevel=1)
    
    def bar():
       warnings.warn("warn to caller", stacklevel=2)
    
    # "default" means 1 warning for each module + line number
    warnings.simplefilter(action="default")
    
    # Warn inside `foo()` because `stacklevel == 1`.
    foo()
    # No warning because this is the same position: inside `foo()`.
    foo()
    
    # Warn here because `stacklevel == 2` (the caller of `foo()`).
    bar()
    # Also warns here because the position is different: same module but different line.
    bar()
tantale commented 1 year ago

Proposal

Regarding the issue you encountered, it can be considered as a bug. To address this, I suggest preparing a fix based on the master branch. The fix involves modifying the @deprecated decorator to include an additional parameter called extra_stacklevel. This parameter, which is an integer with a default value of 0, will be added to the default stacklevel.

By incorporating the extra_stacklevel parameter, you can fine-tune the stacklevel adjustment and ensure that the warning message points to the correct location in the user's code.

With this modification, users can pass an extra_stacklevel value as needed to achieve the desired behavior. For backward compatibility, the default value of 0 ensures that the existing functionality remains intact.

Please let me know if you need any further assistance or clarification. Good luck with the fix!

tantale commented 1 year ago

Acceptance Criteria

tantale commented 1 year ago

Very good job! Congratulations 🎉 Jonas!