[wip]Fix/deprecated 2551 (closes #2551 #2508)

[djarecka]

removing terminal_output and ignore_exceptionwhen creating CommandLine instances. These traits are deprecated in CommandLineInputSpec

Closes #2551
Closes #2508

[effigies]

Ah, terminal_output is still a valid argument to a CommandLine interface and ignore_exception is still valid for BaseInterface, and they do valid things. It's just that they are no longer going to be inputs.

See:

nipype/nipype/interfaces/base/core.py

Lines 173 to 174 in a1ab518

	def __init__(self, from_file=None, resource_monitor=None,
	ignore_exception=False, **inputs):

nipype/nipype/interfaces/base/core.py

Line 906 in a1ab518

def __init__(self, command=None, terminal_output=None, **inputs):

So what's happening at .run(ignore_exception=True) is that it's hitting:

nipype/nipype/interfaces/base/core.py

Line 475 in a1ab518

self.inputs.trait_set(**inputs)

I think the easiest thing would be to add ignore_exception to BaseInterface.run:

def run(self, cwd=None, ignore_exception=None, **inputs):
    if ignore_exception is None:
        ignore_exception = self.ignore_exception

And then just change this line to use the local variable ignore_exception:

nipype/nipype/interfaces/base/core.py

Lines 534 to 535 in a1ab518

	if not self.ignore_exception:
	raise

We should also remove:

nipype/nipype/interfaces/base/core.py

Lines 922 to 925 in a1ab518

	# Attach terminal_output callback for backwards compatibility
	self.inputs.on_trait_change(self._terminal_output_update,
	'terminal_output')

and

nipype/nipype/interfaces/base/core.py

Lines 953 to 955 in a1ab518

	def _terminal_output_update(self):
	self.terminal_output = self.terminal_output

[effigies]

So I would actually undo all of the changes in the current diff, and replace

nipype/nipype/interfaces/base/specs.py

Lines 321 to 328 in a1ab518

	class BaseInterfaceInputSpec(TraitedSpec):
	ignore_exception = traits.Bool(
	False,
	usedefault=True,
	nohash=True,
	deprecated='1.0.0',
	desc='Print an error message instead of throwing an exception '
	'in case the interface fails to run')

with BaseInterfaceSpec = TraitedSpec. And just remove this block:

nipype/nipype/interfaces/base/specs.py

Lines 367 to 381 in a1ab518

	# This input does not have a "usedefault=True" so the set_default_terminal_output()
	# method would work
	terminal_output = traits.Enum(
	'stream',
	'allatonce',
	'file',
	'none',
	deprecated='1.0.0',
	desc=('Control terminal output: `stream` - '
	'displays to terminal immediately (default), '
	'`allatonce` - waits till command is '
	'finished to display output, `file` - '
	'writes output to file, `none` - output'
	' is ignored'),
	nohash=True)

This will require a huge make specs, and probably some doctest updates.

[djarecka]

I'm completely confused. Why do we deprecate these traits, what does it actually mean? we just want to use it differently? only to some methods?

[effigies]

I'm pretty sure this was @oesteban deprecating, but the reason is that they make more sense as attributes than as inputs to be set in the input spec. For instance, nobody passes ignore_exception from one node to another. So the idea is just to remove the traits and anything that tries to set them while keeping the old behavior.

[djarecka]

Ok, so I understand that should remove them from input, but be sure that they can be passed to all methods that are using them and to __init__ (this is already done).

I will revert my changes and try again

[djarecka]

@effigies - I'm not sure about removing ignore_exception and termianl_output from specs.py. I think it might be good to keep it this error for some time.

[djarecka]

I added ignore_exception to run method. I was wondering if we also want to have terminal_output in run (and pass to run_interface etc.) but probably it's enough that it can be set in __init__. Also, shouldn't we have a different default value different, not None? so we can remove this if and we always have self.terminal_output.

[effigies]

I'm not sure about removing ignore_exception and termianl_output from specs.py. I think it might be good to keep it this error for some time.

Sure. It at least gives an informative error. We may want to set a milestone to remove it, just so we don't forget, but that's fine with me. @satra do you agree?

Also, shouldn't we have a different default value different [for terminal_output], not None?

Doing it the way we currently are allows a subclass to set a different default like so:

class NewCommand(CommandLine):
    _terminal_output = 'file_stdout'

If we set a default in __init__, the subclass would have to override __init__ to change the default.

So I would keep it as is.

[satra]

@effigies - fine with me.

[effigies]

Any preferred timeline? 1.1? 1.2? Given that we've only seen two issues, both from internal bugs, not external use of these inputs, I think we can fully remove them after a reasonably short interval.

[satra]

let's do 1.1

[satra]

we can schedule 1.1 for end of june, right after hbm