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

Inline the _cleanup function for the FilePathArtifactLoader. #111

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

maxmynter
Copy link
Collaborator

@maxmynter maxmynter commented Mar 13, 2024

Inline the _cleanup function for the FilePathArtifactLoader.

The arguments of weakref.finalize(obj, func, /, *args, **kwargs) should not own any references to obj. Otherwise, the object is never garbage collected.

Inline the `_cleanup` function for the `FilePathArtifactLoader`.

The arguments of `weakref.finalize(obj, func, /, *args, **kwargs)`
should not own any references to obj. Otherwise, the object is
never garbage collected.

For more info, see https://docs.python.org/3/library/weakref.html#weakref.finalize
@nicholasjng
Copy link
Collaborator

Interesting. Why is it used in the standard lib in tempfile, then? https://github.com/python/cpython/blob/8e45f7d2aecd8820fc2f144f0bb0159351e46ab1/Lib/tempfile.py#L885-L888

Could you maybe try this in a file? Add a print / log statement to self._cleanup() before and after destroying the output of mkdtemp(), both on ordinary exit and with an exception.

Basically, this repro:

# _example.py
from nnbench.types import FilePathArtifactLoader


try:
    f = FilePathArtifactLoader()
    raise RuntimeError("boom")
finally:
    pass

and then check for the prints on both branches.

@nicholasjng
Copy link
Collaborator

Ah, I get it - _cleanup and _rmtree are static, and thus not bound to the instance they are reaping.

@maxmynter maxmynter merged commit 44debd2 into main Mar 14, 2024
5 checks passed
@nicholasjng nicholasjng deleted the fix-finalizer branch March 18, 2024 14:45
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