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

[ENH] Stacklevel offset #68

Closed
coroa opened this issue Jul 6, 2023 · 4 comments · Fixed by #69
Closed

[ENH] Stacklevel offset #68

coroa opened this issue Jul 6, 2023 · 4 comments · Fixed by #69

Comments

@coroa
Copy link
Contributor

coroa commented Jul 6, 2023

Over at coroa/pandas-indexing#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 tantale self-assigned this Jul 8, 2023
@tantale tantale added this to the 1.2.15 milestone Jul 8, 2023
@tantale
Copy link
Collaborator

tantale commented Jul 8, 2023

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
Copy link
Collaborator

tantale commented Jul 8, 2023

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
Copy link
Collaborator

tantale commented Jul 8, 2023

Acceptance Criteria

  • Problem Resolution: The fix should effectively resolve the reported problem.
  • No Regressions: The fix should not introduce new issues or regressions in the existing codebase.
  • Adherence to Best Practices: The fix should follow coding standards (isort, black).
  • The fix must be compatible to all supported versions including Python 2.7.
  • Sufficient Testing: Adequate testing should be performed to verify the proper functioning of the fix.
  • Comprehensive Documentation: Clear and concise documentation accompanying the fix is essential.

@tantale
Copy link
Collaborator

tantale commented Jul 9, 2023

Very good job! Congratulations 🎉 Jonas!

coroa added a commit to coroa/pandas-indexing that referenced this issue Jul 11, 2023
Deprecate idx in favour of pix. Deprecation warning is not yet triggered automatically due to laurent-laporte-pro/deprecated#68 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants