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

UnboundLocalError appeared in lazy-loader 0.4 #136

Closed
Bingshuang0u0 opened this issue Dec 19, 2024 · 5 comments · Fixed by #137
Closed

UnboundLocalError appeared in lazy-loader 0.4 #136

Bingshuang0u0 opened this issue Dec 19, 2024 · 5 comments · Fixed by #137

Comments

@Bingshuang0u0
Copy link

I was using librosa.load() which uses lazy-loader, and the error below appeared.

Error Message:

UnboundLocalError: local variable 'parent' referenced before assignment

The Code Where The Error Appeared:

try:
    parent = inspect.stack()[1]
    frame_data = {
        "filename": parent.filename,
        "lineno": parent.lineno,
        "function": parent.function,
        "code_context": parent.code_context,
    }
    return DelayedImportErrorModule(
        frame_data,
        "DelayedImportErrorModule",
        message=not_found_message,
    )
finally:
    del parent

This was mentioned in issue#79, but it happened again in version 0.4

Environment:

Python 3.9.13
lazy-loader 0.4

@stefanv
Copy link
Member

stefanv commented Dec 19, 2024

Thanks for the report @Bingshuang0u0; I'll take a look.

@effigies
Copy link
Collaborator

I don't fully understand what this try-finally block is doing for us. It will delete before returning or raising, but I think the following should get you the same result, while allowing inspect.stack()[1] to fail:

# Can raise without triggering finally
parent = inspect.stack()[1]
# Can raise AttributeError
try:
    frame_data = {
        "filename": parent.filename,
        "lineno": parent.lineno,
        "function": parent.function,
        "code_context": parent.code_context,
    }
finally:
    del parent
# Parent is deleted, so we can safely raise anything here
return DelayedImportErrorModule(
    frame_data,
    "DelayedImportErrorModule",
    message=not_found_message,
)

That said, I don't think any of those attribute lookups on parent can fail. I think we could drop the try-finally and just:

parent = inspect.stack()[1]
frame_data = {
    "filename": parent.filename,
    "lineno": parent.lineno,
    "function": parent.function,
    "code_context": parent.code_context,
}
del parent
return DelayedImportErrorModule(
    frame_data,
    "DelayedImportErrorModule",
    message=not_found_message,
)

@dschult
Copy link
Contributor

dschult commented Dec 19, 2024

I agree that it is very unlikely that an exception will occur during the construction of the frame_data dict. It would only happen if the object on the inspect.stack was missing one of these attributes. That would mean something is seriously wrong.

Blame shows the finally clause was added in #4 presumably to remove the parent pointer to the object in inspect.stack()[1] when we return the DelayedImportErrorModule, or when an exception occurs while constructing that module. When reading the code today, I assumed the try/finally was looking to handle the case when an error occurs creating the module.

But of course the try is a few lines above where it needs to be for that. We definitely don't want the try to include errors during creating parent if the finally is deleting parent.

I don't know any reason to worry about deleting variable pointers like parent before returning other than garbage collection. And I'm not an expert on garbage collection in Python. But however it is handled, deleting parent before constructing and returning should have the same effect as the finally clause. And it removes the problem here where creating parent is inside the try/finally.

So, I think @effigies suggestion of removing the try/finally and adding del parent just before returning is a good solution.

@stefanv
Copy link
Member

stefanv commented Dec 19, 2024

Great, thanks for the suggestions y'all. Did anyone make a PR, or should I go ahead?

@effigies
Copy link
Collaborator

Sure, just opened #137.

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 a pull request may close this issue.

4 participants