[ENH] Revising use of subprocess.Popen #2289
Conversation
Make sure everything is tidied up after using Popen.
nipype/interfaces/base.py
Outdated
| stderr=sp.PIPE, | ||
| ) | ||
| o, e = proc.communicate() | ||
| return o |
There was a problem hiding this comment.
This is an API change. Even if not used internally, someone may depend on this method. Does nipype have a process for deprecating?
There was a problem hiding this comment.
Maybe we should start using https://pypi.python.org/pypi/deprecation ?
nipype/interfaces/base.py
Outdated
|
|
||
| runtime.returncode = proc.returncode | ||
| proc.terminate() # Ensure we are done | ||
| gc.collect() # Force GC for a cleanup |
There was a problem hiding this comment.
Goal here is just to clean up proc internals, not stdout/stderr, etc?
nipype/interfaces/base.py
Outdated
| proc = sp.Popen( | ||
| cmd(name), stdout=sp.PIPE, stderr=sp.PIPE, shell=True, | ||
| env=environ, close_fds=True) | ||
| o, e = proc.communicate() |
There was a problem hiding this comment.
I'd replace e with _, since it's unused.
| @@ -1452,12 +1448,28 @@ def _process(drain=0): | |||
| result['merged'] = result['stdout'] | |||
| result['stdout'] = [] | |||
| else: | |||
| proc.communicate() # Discard stdout and stderr | |||
| stdout, stderr = proc.communicate() | |||
There was a problem hiding this comment.
Maybe make these stdoutstr, stdinstr, to be consistent with if output.startswith('file'), so that the same del statement will work in all cases.
Alternately, it might be simplest to just run gc.collect() in CommandLine._run_interface, after this function is called and everything is allowed to go out of scope.
nipype/interfaces/base.py
Outdated
| result['stdout'] = read_stream(stdoutstr, logger=iflogger) | ||
| result['stderr'] = read_stream(stderrstr, logger=iflogger) | ||
| del stdoutstr | ||
| del stderrstr |
There was a problem hiding this comment.
Sorry, meant for these to replace the del stdout and del stderr below, since stdoutstr and stderrstr can be created in the other branch.
I guess, though, that those only exist under certain sub-branches. Not sure how you want to go about this.
nipype/interfaces/base.py
Outdated
| _ = getattr(dup, key) | ||
| except: | ||
| pass | ||
| # for key in self.copyable_trait_names(): |
There was a problem hiding this comment.
dynamic traits won't work without these. i'm not sure we have a test for it but this is the relevant issue and PR.
nipype/interfaces/base.py
Outdated
| @@ -1620,6 +1642,8 @@ def _get_environ(self): | |||
| return getattr(self.inputs, 'environ', {}) | |||
|
|
|||
| def version_from_command(self, flag='-v'): | |||
| iflogger.warning('version_from_command member of CommandLine was ' | |||
| 'Deprecated in nipype-1.0.0 and deleted in 2.0.0') | |||
There was a problem hiding this comment.
we should perhaps keep this in the 1.0 branch so deleted in 1.1
This PR:
PopeninCommandLineInterfaceensuring they are terminated and garbage-collected.interfaces.base)interfaces.base)interfaces.base)engine.workflowsandplugins.multiproc.get_dependenciesand executes it only iff the optionexecution. get_linked_libsisTrue(which is the default value).