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

remove del operator on PhaseAnalyzer #634

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

richardjgowers
Copy link
Contributor

fixes #633

the __del__ doesn't do anything except delete attributes, so isn't necessary

fixes choderalab#633

it doesn't do anything except delete attributes, so isn't necessary
@ijpulidos ijpulidos self-requested a review November 26, 2022 18:23
@mikemhenry
Copy link
Contributor

I haven't looked at this bit of code lately, maybe @ijpulidos could check, but __del__ does call self.clear(), which does some cache clearing stuff

@richardjgowers
Copy link
Contributor Author

I did a quick scan, there's no explicit .close() or similar calls, it's just removing references from the _cache dict, which is what Python garbage collection should also be doing. I didn't have time to go through the git history to see if there used to be a file handle held....

@ijpulidos
Copy link
Contributor

@richardjgowers thanks for reporting the issue and contributing the possible solution.

I tend to agree with your remarks, the Python garbage collector should already be doing this. Nevertheless, I would like to have a reproducible example/snippet where the issue appears, it doesn't have to be a simple one, just something I can use to reproduce the issue and go from there. If you have one I can use that would be very helpful. Thanks!

@mikemhenry
Copy link
Contributor

We should probably catch the AttributeError in _invalidate_cache_values since (from https://stackoverflow.com/a/1481512) mentions

However, there are valid use cases for del: e.g. if an object X references Y and also keeps a copy of Y reference in a global cache (cache['X -> Y'] = Y) then it would be polite for X.del to also delete the cache entry.

Which looks like what we are doing here.

Also from the docs it sounds like __del__ " [is] called when the instance is about to be destroyed" so we really just be using context mangers if cleanup is important with an __exit__ (or finally block) since it might not get called until the intrepter exits.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #634 (229053f) into main (6cef59b) will decrease coverage by 0.01%.
The diff coverage is n/a.

@richardjgowers
Copy link
Contributor Author

Aha ok. For context, I think I was getting this error when I was trying to create a PhazeAnalyzer and it was failing in __init__ which is why you end up trying to delete an attribute that should probably always exist.

@mikemhenry mikemhenry enabled auto-merge (squash) August 22, 2023 21:46
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.

PhaseAnalyzer del causing error.
3 participants