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 memory leak in cached_property backport #2011

Merged
merged 3 commits into from Feb 8, 2022

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented Feb 7, 2022

Closes #2008 by replacing the existing backport with the cached_property implementation from the Python-3.8 repository.

@sfinkens sfinkens self-assigned this Feb 7, 2022
@sfinkens
Copy link
Member Author

sfinkens commented Feb 7, 2022

Example with one day of ABI data.

memory_leak_abi_l1b(2)

Black: Current implementation with Python-3.7
Blue: Fix with Python-3.7

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #2011 (6c9cf22) into main (34bd429) will decrease coverage by 0.01%.
The diff coverage is 83.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2011      +/-   ##
==========================================
- Coverage   93.56%   93.55%   -0.02%     
==========================================
  Files         281      282       +1     
  Lines       41480    41538      +58     
==========================================
+ Hits        38811    38860      +49     
- Misses       2669     2678       +9     
Flag Coverage Δ
behaviourtests 4.77% <13.20%> (+<0.01%) ⬆️
unittests 94.08% <83.01%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/_compat.py 75.00% <75.00%> (-6.82%) ⬇️
satpy/tests/test_compat.py 100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_abi_l2_nc.py 100.00% <0.00%> (ø)
satpy/readers/abi_l2_nc.py 96.00% <0.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34bd429...6c9cf22. Read the comment docs.

@djhoese
Copy link
Member

djhoese commented Feb 7, 2022

Where does this solution come from? Stackoverflow? Official CPython docs somewhere?

@djhoese
Copy link
Member

djhoese commented Feb 7, 2022

If you update line 267 in doc/source/conf.py to say:

    'numpy': ('https://numpy.org/doc/stable', None),

it should fix the build_website failure. Numpy updated their documentation structure.

@sfinkens
Copy link
Member Author

sfinkens commented Feb 7, 2022

Where does this solution come from? Stackoverflow? Official CPython docs somewhere?

It was an stackoverflow answer that pointed me to the CPython repository: https://stackoverflow.com/a/58592597/5703449

@sfinkens
Copy link
Member Author

sfinkens commented Feb 7, 2022

'numpy': ('https://numpy.org/doc/stable', None),

Done, thanks!

@coveralls
Copy link

coveralls commented Feb 7, 2022

Coverage Status

Coverage decreased (-0.01%) to 94.012% when pulling 6c9cf22 on sfinkens:fix-cached-property-backport into e27414f on pytroll:main.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

This looks good to me. It is a much more involved solution than I thought it was going to be. I think given that we are removing Python 3.7 support I think we should merge this, make a release, then immediately remove this as part of a PR to drop Python 3.7 support. Thoughts @sfinkens @mraspaud @pnuu ?

Oops @gerritholl you too, thoughts?

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM as a patch for the last release supporting python 3.7 :)

@mraspaud mraspaud merged commit 94fc4f7 into pytroll:main Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

abi_l1b reader leaks memory in Python-3.7
4 participants