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

Refer to a function caller not wrapt/wrappers.py #13

Merged
merged 1 commit into from Nov 11, 2019

Conversation

MHendricks
Copy link
Contributor

When using @deprecated.deprecated on a module function, the raised warning reports the problem in "wrapt\wrappers.py" instead of the file that called the decorated function.

test.py:

from deprecated import deprecated
@deprecated
def foo1():
    pass

class Test(object):
    @deprecated
    def foo2(self):
        pass

if __name__ == '__main__':
    foo1()

    t = Test()
    t.foo2()

When running python test.py with the un-patched deprecated module results in:

C:\Program Files\Python37\lib\site-packages\wrapt\wrappers.py:564: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  args, kwargs)
C:\Program Files\Python37\lib\site-packages\wrapt\wrappers.py:603: DeprecationWarning: Call to deprecated method foo2.
  args, kwargs)

This makes it hard to track down the code that is calling the deprecated method.

With the patch:

test.py:12: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
test.py:15: DeprecationWarning: Call to deprecated method foo2.
  t.foo2()

This makes it easy to find and fix the call to a deprecated method and I assume is the desired behavior.

@MHendricks
Copy link
Contributor Author

MHendricks commented Nov 7, 2019

Hmm, I guess windows has a different stacklevel than linux does. I guess I'll need to figure that out.

@coveralls
Copy link

coveralls commented Nov 8, 2019

Coverage Status

Coverage decreased (-1.7%) to 94.444% when pulling cd80503 on blurstudio:stacklevel-fix into 6145b59 on tantale:master.

@MHendricks
Copy link
Contributor Author

I'm not sure what is different between the appveyor windows python and my windows python setups, but for some reason it changes the stacklevel requirement.

Windows 7 Python 2.7.3 requires a stack level of 3.
Windows 10 Python 2.7.16 requires a stack level of 3.
Centos 7.6.1810 Python 2.7.5 requires a stack level of 2.
The tests seem to require a stack level of 2 except for python 3.4 64bit, and Seem to be running on windows.

test.py:

from deprecated import deprecated
@deprecated
def foo1():
    pass

if __name__ == '__main__':
    foo1()
diff --git a/deprecated/classic.py b/deprecated/classic.py
index f214ec0..9086e91 100644
--- a/deprecated/classic.py
+++ b/deprecated/classic.py
@@ -235,6 +235,10 @@ def deprecated(*args, **kwargs):
                     else:
                         stacklevel = 2
                     warnings.warn(msg, category=category, stacklevel=stacklevel)
+                    import traceback
+                    print('-'*50)
+                    traceback.print_stack()
+                    print('-'*51)
                 return wrapped_(*args_, **kwargs_)

             return wrapper_function(wrapped)

If I make this change to show the stack when the warning is raised I get different results.
Windows:

test.py:7: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
--------------------------------------------------
  File "test.py", line 7, in <module>
    foo1()
  File "C:\temp\depr_test\lib\site-packages\wrapt\wrappers.py", line 564, in __call__
    args, kwargs)
  File "c:\blur\dev\deprecated\deprecated\classic.py", line 240, in wrapper_function
    traceback.print_stack()
---------------------------------------------------

Centos:

test.py:7: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
--------------------------------------------------
  File "test.py", line 7, in <module>
    foo1()
  File "/home/ad.blur.com/mikeh/checkouts/deprecated/deprecated/classic.py", line 240, in wrapper_function
    traceback.print_stack()
---------------------------------------------------

I think this may be a issue with wrapt, but so far I've been unable to follow the code of wrapt to figure out how it could be evaluating differently in the different environments.

Do you have any suggestions on a better way to ensure the correct stacklevel is used in the deprecation warning, or any idea why the appveyor tests seem to have different results?

@tantale
Copy link
Owner

tantale commented Nov 8, 2019

Thank you for your contribution.

On Windows 7 Professional, I have:

(py-tmp) D:\Laurent\Projets\workspace\tmp>python --version
Python 2.7.16

(py-tmp) D:\Laurent\Projets\workspace\tmp>python test.py
test.py:17: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
test.py:19: DeprecationWarning: Call to deprecated method foo2.
  t.foo2()

So, I can't reproduce the problem on this platform.

But, you are right, the problem comes from the Wrapt library which perform some code modification at runtime, and uses C extension…

@tantale
Copy link
Owner

tantale commented Nov 8, 2019

Can you try to add a unit test which check that the __file__ is in the stack trace of a decorated function?

Here is a starting point:

import traceback
import wrapt

@wrapt.decorator
def pass_through(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)

@pass_through
def function():
    stack = traceback.format_stack()
    assert __file__ in stack[-1]

if __name__ == '__main__':
    function()

That way we can make sure that this is a Wrapt problem and find a way to work aroud it…

tests/test.py Outdated


@pass_through
def file_in_stack():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the py34 test environment has the same problem as I'm seeing on my system. test output It was the only test environment that replicated my problem in my original pull request test. I'm not sure what is different about that environment.

py34 replicates the problem in the pass_through test:
https://ci.appveyor.com/project/tantale/deprecated/builds/28725583/job/w3a7smf17v5awg2h#L161

py35 (or any other) does not replicate the problem in the pass_through test:
https://ci.appveyor.com/project/tantale/deprecated/builds/28725583/job/jv1wm6n7x3b30fcg#L156

I forced the new tests to always fail so we could compare the tracebacks. Here is the py34 stack reported reported when using the pass_through decorator.

E           File "C:\projects\deprecated\tests\test.py", line 46, in test_file_in_wrapt_stack
E             file_in_stack()
E           File "c:\projects\deprecated\.tox\py34\lib\site-packages\wrapt\wrappers.py", line 564, in __call__
E             args, kwargs)
E           File "C:\projects\deprecated\tests\wrapt_decorators.py", line 8, in pass_through
E             return wrapped(*args, **kwargs)
E           File "C:\projects\deprecated\tests\test.py", line 36, in file_in_stack
E             stack = traceback.format_stack(limit=10)

The py35 output:

E           File "c:\projects\deprecated\.tox\py35\lib\site-packages\_pytest\python.py", line 170, in pytest_pyfunc_call
E             result = testfunction(**testargs)
E           File "C:\projects\deprecated\tests\test.py", line 46, in test_file_in_wrapt_stack
E             file_in_stack()
E           File "C:\projects\deprecated\tests\wrapt_decorators.py", line 8, in pass_through
E             return wrapped(*args, **kwargs)
E           File "C:\projects\deprecated\tests\test.py", line 36, in file_in_stack
E             stack = traceback.format_stack(limit=10)

I also added a similar unit test using the deprecated decorator.

@MHendricks
Copy link
Contributor Author

I figured it out. We don't have Microsoft Visual C++ Compiler for Python 2.7 installed.

Wrapt pip installs even if it can't compile the _wrappers.pyd file. After installing the C++ compiler and reinstalling wrapt, I'm getting the expected stack level you are getting.

You should be able to replicate the problem yourself by renaming your wrapt\_wrappers.pyd file. I've updated the merge request with a way to detect and correctly set the stacklevel.

Given that the py34 test had the problem I'm guessing it doesn't have _wrappers.pyd on AppVeyor.

I haven't been able to figure out a way make a unit test that covers both if _wrappers.pyd exists and doesn't exist, but I'll think about it over the weekend. Let me know if you have any suggestions or don't think its worth testing.

@tantale tantale merged commit 4fff342 into tantale:master Nov 11, 2019
tantale added a commit that referenced this pull request Feb 23, 2020
…ecorating a class if wrapt does not have the compiled c extension.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants