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

Make DataStoreObservation properties less lazy #1226

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented Dec 1, 2017

This PR should fix the immediate problem mentioned in #1213, i.e. avoid memory consumption from growing endlessly as more observations are processed. In principle this could mean performance regressions that this change leads to events or IRFs being read from disk more than once. In practice I don't think we do this anywhere, i.e. this change is good.

All tests pass for me locally. Otherwise I didn't test this at all, i.e. I didn't check if it actually solves the memory issue. @adonath - assigning to you.

@cdeil cdeil added the bug label Dec 1, 2017

@cdeil cdeil added this to the 0.7 milestone Dec 1, 2017

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Dec 1, 2017

Member

I just ran the benchmarks and can confirm that this resolves the memory issue.

Before there was the expected linear increase in memory usage, increasing from ~250mb to ~700mb (for 100 tested runs):
Before

After the memory usage stays constant at ~250mb:
After

Member

adonath commented Dec 1, 2017

I just ran the benchmarks and can confirm that this resolves the memory issue.

Before there was the expected linear increase in memory usage, increasing from ~250mb to ~700mb (for 100 tested runs):
Before

After the memory usage stays constant at ~250mb:
After

@adonath

I confirm that this resolves #1213. I have no further comments.

@adonath

adonath approved these changes Dec 1, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 1, 2017

Member

@adonath - Thanks for doing all the real work here, i.e. setting up the benchmarks!

Member

cdeil commented Dec 1, 2017

@adonath - Thanks for doing all the real work here, i.e. setting up the benchmarks!

@cdeil cdeil merged commit 2a3b008 into gammapy:master Dec 1, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment