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

Fix deprecation warning stack level #1058

Merged
merged 3 commits into from
Aug 12, 2021
Merged

Fix deprecation warning stack level #1058

merged 3 commits into from
Aug 12, 2021

Conversation

bfis
Copy link
Contributor

@bfis bfis commented Aug 11, 2021

The current warning(s) emitted by deprecate (currently only used in fill_none) are not too helpful since they show a position in the deprecate helper function.

By using the stacklevel parameter of warnings.warn we can adjust this to show the line where the (marked as deprecated) function was called from i.e. usually somewhere in the users code.

@jpivarski
Copy link
Member

That sounds good. The original idea was that if you wanted to know where in your code it's happening, you'd turn the warning into an exception (that's why instructions are given in the warning text), and the exception would give you a full stack trace.

Will stacklevel=3 always be exactly the right place? If I'm counting right, stacklevel=1 is in ak._util.deprecate (the least useful place), stacklevel=2 is in the ak.* function that called it, and stacklevel=3 is in the user's code that called that. But now we'll have to be careful that ak._util.deprecate is always only called directly in an ak.* function, not inside a function called by an ak.* function.

It's unfortunate that Python doesn't know that we're drawing a dotted line between the Awkward library code and user code. Or is there an alternative that says, "climb the stack trace until a function's __module__ doesn't satisfy __module__.startswith("awkward.")?" It doesn't look like there is...

We use ak._util.deprecate infrequently, so maybe the thing to do is to just include a comment warning/reminding us about this constraint. Or maybe stacklevel should be passed into the ak._util.deprecate function as a required argument, so that we can make it stacklevel=4 if we put it in something nested.

@bfis
Copy link
Contributor Author

bfis commented Aug 12, 2021

I've added a stacklevel kwarg to ak._util.deprecate that functions similar to warnings.warn: A stacklevel of 1 would report the position of the ak._util.deprecate call. However, I've set the default to 2 since we are mainly interested in reporting the position within the user's code. For cases an internal function (i.e. a function indirectly called by the user) is deprecated we can properly modify the stacklevel (e.g. set it to 3), in so far it is possible/deterministic. Other cases could be handled by a generic implementation that crawls the stack, as you have already outlined above - however, for now, this would be overkill i think.

@jpivarski
Copy link
Member

Other cases could be handled by a generic implementation that crawls the stack, as you have already outlined above - however, for now, this would be overkill i think.

I agree. As an internal function, we're always the ones who will be calling it, so we'll just be careful to set the stacklevel appropriately.

This looks done; I'll be merging when the tests all pass.

@jpivarski jpivarski merged commit 42327ff into scikit-hep:main Aug 12, 2021
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.

2 participants