-
Notifications
You must be signed in to change notification settings - Fork 130
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
ENH: Revise plugin and workflow initialization #983
Conversation
_plugin = { | ||
"plugin": Plugin(plugin_args=config.nipype.plugin_args), | ||
} | ||
gc.collect() |
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.
What is this collecting that doesn't get cleaned up on L95?
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 don't know what L95 could be possibly be collecting so... this is to say, I'm happy either way and maybe there are some unused variables in the plugin implementation that this would catch. Honestly, IDK. Happy to remove if it feels superfluous.
Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
Hello @oesteban, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2022-04-20 19:18:45 UTC |
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.
Looks good. I think you could probably safely remove the garbage collection calls at this point, but no harm in keeping them in.
It seems that confining the workflow creation within an isolated process does not meaningfully help with the VM allocation while increasing the complexity horribly.
On the other hand, the config file has been made considerably more lightweight by using importlib metadata to check versions instead of importing packages, and the plugin instance (MultiProc and LegacyMultiProc) are now created at earlier states so workers are initialized with smaller memory fingerprint.