-
Notifications
You must be signed in to change notification settings - Fork 19
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
MultiFab Fixture Cleanup via FabArray::clear
#214
Conversation
Using a context manager and calling clear ensures that we will not hold device memory anymore once we hit `AMReX::Finalize`, even in the situation where an exception is raised in a test. This avoids segfaults for failing tests.
Clear out memory safely on runtime errors.
del self.mfab | ||
|
||
with MfabContextManager() as mfab: | ||
yield mfab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be simplified?
def mfab(boxarr, distmap, request):
num_components = request.param[0]
num_ghost = request.param[1]
try:
mfab = amr.MultiFab(boxarr, distmap, num_components, num_ghost)
mfab.set_val(0.0, 0, num_components)
yield mfab
finally:
mfab.clear()
del mfab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my knowledge, that will cache the MultiFab between runs and break out memory pool assumptions...
Worth to try again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. The fixture is marked function scope, so it shouldn't cache a value. Is it pytest that's caching the value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so... maybe I misunderstood something when caching kicks in.
def __enter__(self): | ||
num_components = request.param[0] | ||
num_ghost = request.param[1] | ||
self.mfab = amr.MultiFab(boxarr, distmap, num_components, num_ghost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pylint
would probably create warnings for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh really? Linting still coming in #208 😅
CodeQL did not yet complain, what's to be avoided here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pylint will complain about attributes being declared/created in a non-init function.
@sayerhs I wonder if I shall just merge this for now and refactor it later, since it fixes issues we see on Perlmutter...? |
Using a context manager and calling clear ensures that we will not hold device memory anymore once we hit
AMReX::Finalize
, even in the situation where an exception is raised in a test. This avoids segfaults for failing tests.Related to #81