Avoid calling cleanup from destructor #168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
This comment has been minimized.
This comment has been minimized.
WeinaJi
previously approved these changes
May 8, 2024
There was a problem hiding this comment.
LGTM! Thanks!
@cattabiani Once this change is deployed, in multiscale_run you will have to remove cleanup_atexit from calling neurodamus.
WeinaJi
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
from
b4fd6ae to
a636f3d
Compare
This comment has been minimized.
This comment has been minimized.
|
Shall we merge this PR ? |
Looks good to me! |
jorblancoa
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
from
a636f3d to
a541267
Compare
This comment has been minimized.
This comment has been minimized.
jorblancoa
previously approved these changes
May 15, 2024
This comment has been minimized.
This comment has been minimized.
|
Tests just failed because pytest-forked depends on py.process which went away. pytest-dev/pytest-forked#88 |
This comment has been minimized.
This comment has been minimized.
ferdonline
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
from
5c2e841 to
fa58d2e
Compare
This comment has been minimized.
This comment has been minimized.
ferdonline
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
2 times, most recently
from
8e1fcb6 to
b7856b9
Compare
This comment has been minimized.
This comment has been minimized.
ferdonline
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
from
b7856b9 to
5f8f84f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ferdonline
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
from
5f8f84f to
5e2fe82
Compare
This comment has been minimized.
This comment has been minimized.
ferdonline
force-pushed
the
enh/leite/no-cleanup-in-dtor
branch
2 times, most recently
from
11423ff to
be0b856
Compare
|
Logfiles from GitLab pipeline #211543 (:no_entry:) have been uploaded here! Status and direct links: |
jorblancoa
approved these changes
May 16, 2024
atemerev
pushed a commit
that referenced
this pull request
May 24, 2024
## Context Neurodamus used to ensure `cleanup` was called by calling it from the destructor of the `Neurodamus` class. However, depending on the python version, this could lead to crashes as some objects from `Node` eventually were already destroyed. To workaround this problem, at some point we introduced the `cleanup_atexit` parameter which would skip over this procedure, and notably multiscale_run was using it. However this was not a solution as cleanup was not being called. ## Scope This PR fixes this long standing issue. Cleanup is now called directly from the top-level `run()` function. It can still be avoided if the user really wants to keep the data, with the parameter `cleanup=False` ## Testing CI is happy. ## Review * [x] PR description is complete * [x] Coding style (imports, function length, New functions, classes or files) are good
WeinaJi
pushed a commit
that referenced
this pull request
Oct 14, 2024
## Context Neurodamus used to ensure `cleanup` was called by calling it from the destructor of the `Neurodamus` class. However, depending on the python version, this could lead to crashes as some objects from `Node` eventually were already destroyed. To workaround this problem, at some point we introduced the `cleanup_atexit` parameter which would skip over this procedure, and notably multiscale_run was using it. However this was not a solution as cleanup was not being called. ## Scope This PR fixes this long standing issue. Cleanup is now called directly from the top-level `run()` function. It can still be avoided if the user really wants to keep the data, with the parameter `cleanup=False` ## Testing CI is happy. ## Review * [x] PR description is complete * [x] Coding style (imports, function length, New functions, classes or files) are good
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Neurodamus used to ensure
cleanupwas called by calling it from the destructor of theNeurodamusclass.However, depending on the python version, this could lead to crashes as some objects from
Nodeeventually were already destroyed.To workaround this problem, at some point we introduced the
cleanup_atexitparameter which would skip over this procedure, and notably multiscale_run was using it. However this was not a solution as cleanup was not being called.Scope
This PR fixes this long standing issue. Cleanup is now called directly from the top-level
run()function. It can still be avoided if the user really wants to keep the data, with the parametercleanup=FalseTesting
CI is happy.
Review