Calculate correct stack level for deprecation warnings to show them to the user #3569
Conversation
…o the user. Generating deprecation warnings with the correct stacklevel is tricky to get right. Up to now, users have not been seeing these deprecation warnigns because they had incorrect stacklevels. See python/cpython#68493 and python/cpython#67998 for good discussions, as well as ipython/ipython#8478 (comment) and ipython/ipython#8480 (comment) for more context. We see these issues directly. Normally stacklevel=2 might be enough to target the caller of a function. However, in our deprecation warning in the __init__ method, each level of subclasses adds one stack frame, so depending on the subclass, the appropriate stacklevel is different. Thus this PR implements a trick from a discussion in Python to calculate the first stack frame that is external to the ipywidgets library, and use that as the target stack frame for the deprecation warning.
|
|
…vel deeper than it is being used. Also, make the stack level calculator private until there is a need to make it public.
…vel deeper than it is being used.
Also, make the stack level calculator private until there is a need to make it public.
…venience in the default, and adjust default deprecation stacklevel Also put in workaround if sys._getframe doesn't work
|
I was about to push my tests and another fix as well - I'll work on incorporating your changes, especially your path separator fix - thanks! |
|
I see you are still pushing tests. Please let me know when you are done and I'll work on merging my tests and your tests. |
|
I'm done, but please look at the bit there I removed the |
|
And sorry for not commenting that I was working on it! |
I think it's more that stacklevel 0 is actually inside the warn function: https://github.com/python/cpython/blob/main/Lib/warnings.py#L298-L310 |
…ized Because deprecated attributes of the Widget class are static attributes, they will be accessed when initializing this class by traitlets. In that case, since a user did not explicitly try to use these attributes, we do not want to throw a deprecation warning. So we check if the thing calling these static properties is one of the known initialization functions in traitlets.
|
@vidartf - I merged your changes in and I think this is ready for another review from you. |
There was a problem hiding this comment.
Nice work, also on the traitlet path (a complexity I guess I introduced), I've also checked it on the python repl:
>>> w.Text(description_tooltip="fda")
<stdin>:1: DeprecationWarning: the description_tooltip argument is deprecated, use tooltip instead
Text(value='', tooltip='fda')
👍
Generating deprecation warnings with the correct stacklevel is tricky to get right. Up to now, users have not been seeing these deprecation warnings because we had incorrect stacklevels. See python/cpython#68493 and python/cpython#67998 for good discussions, as well as ipython/ipython#8478 (comment) and ipython/ipython#8480 (comment) for more context.
We see these issues directly. Normally stacklevel=2 might be enough to target the caller of a function. However, in our deprecation warning in the init method, each level of subclasses adds one stack frame, so depending on the subclass, the appropriate stacklevel is different. Thus this PR implements a trick from a discussion in Python to calculate the first stack frame that is external to the ipywidgets library, and use that as the target stack frame for the deprecation warning.
This code should now produce 6 deprecation warnings in Jupyter Notebook or JupyterLab, whereas before it was producing no visible deprecation warnings.