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

Set appropriate stacklevel for userwarnings #2405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hmaarrfk
Copy link
Contributor

I typically try to set the stacklevel to point to the correct location where the user can fix the warning in their code.

Previously, the stack level would point the user to some deep deep internal method for vispy.

The _stacklevel_increment is designed to be an internal (vispy-only) parameter that allows passing the warning upward to the user's originating call.

@hmaarrfk
Copy link
Contributor Author

Truthfully, I patched up until i found the warning I was getting, and stopped. This could be more extensive, but I probably don't have the time to go through this all.

@djhoese
Copy link
Member

djhoese commented Oct 17, 2022

Not going to lie (and I say this because you've been involved with vispy for so long), this is ugly. That said, I understand the need for it.

What are the difference cases for the different stack levels that _stacklevel_increment would be 0 or 1 or 2 (I assume we don't need more than 2)?

If we assume users aren't using gloo-level classes directly then would _stacklevel_increment being hardcoded to 1 serve most purposes so we can then get rid of the kwarg?

@brisvag thoughts?

@@ -472,9 +478,11 @@ class Texture1D(BaseTexture):

def __init__(self, data=None, format=None, resizable=True,
interpolation=None, wrapping=None, shape=None,
internalformat=None, resizeable=None):
internalformat=None, resizeable=None,
_stacklevel_increment=0):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can remove them from here, but the more you try to delegate, the more you will find yourself needing these.

I know it is ugly, but the thing you really want to avoid is somebody filtering away all the warnings, because you point them inside a helper function.

Filter all is really easy to write.....

@brisvag
Copy link
Collaborator

brisvag commented Oct 18, 2022

Yeah, I'm not a huge fan either... Is it problematic to just always point the max amount of levels up in the "public" api?

@hmaarrfk
Copy link
Contributor Author

Yeah, I'm not a huge fan either... Is it problematic to just always point the max amount of levels up in the "public" api?

You can do that if you can figure out where the public API boundary lives.

@aganders3
Copy link
Contributor

Is this idea too insane? It looks for the nearest stack frame that points to a file outside the vispy search path. It takes ~0.3 ms for the first execution and is ~10x faster for subsequent calls.

def _get_user_code_stacklevel():
    import vispy
    import traceback
    stack = traceback.extract_stack()
    for level, frame in enumerate(stack[::-1]):
        if not frame.filename.startswith(vispy.__spec__.submodule_search_locations[0]):
            break
    return level

@djhoese
Copy link
Member

djhoese commented Jun 24, 2023

Besides whether it is valid or not, would it make sense to remove the "not" and return the level inside the if statement?

@aganders3
Copy link
Contributor

You mean walk the stack in the other direction?

@djhoese
Copy link
Member

djhoese commented Jun 24, 2023

No. I mean I don't think "break" is necessary. You could just put the return there instead of breaking. I'm not sure what I was thinking when I said to remove "not", ignore that.

@aganders3
Copy link
Contributor

Ohh, I see. Yes. You're right. The only difference is if it never finds a stack frame with a file outside the vispy path, it would return None where as-is it would return the max stack level. I don't think that would generally happen though because as far as I know vispy is always used as a library not an application.

@djhoese
Copy link
Member

djhoese commented Jun 24, 2023

Yeah, leave it as is.

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

4 participants