fix: use matlab_cmd provided as input closes #2442 #2452
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,28 +126,29 @@ def scans_for_fnames(fnames, keep4d=False, separate_sessions=False): | |
|
|
||
| class Info(PackageInfo): | ||
| """Handles SPM version information | ||
|
|
||
| If you use `SPMCommand.set_mlab_paths` to set alternate entries for | ||
| matlab_cmd, paths, and use_mcr, then you will need to use the same entries | ||
| to any call in the Info class to maintain memoization. Otherwise, it will | ||
| default to the parameters in the `getinfo` function below. | ||
| """ | ||
| _path = None | ||
| _name = None | ||
| _command = None | ||
| _paths = None | ||
|
There was a problem hiding this comment. shouldn't you also add |
||
|
|
||
| @classmethod | ||
| def path(klass, matlab_cmd=None, paths=None, use_mcr=None): | ||
| klass.getinfo(matlab_cmd, paths, use_mcr) | ||
| return klass._path | ||
|
|
||
| @classmethod | ||
| def version(klass, matlab_cmd=None, paths=None, use_mcr=None): | ||
| klass.getinfo(matlab_cmd, paths, use_mcr) | ||
| return klass._version | ||
|
|
||
| @classmethod | ||
| def name(klass, matlab_cmd=None, paths=None, use_mcr=None): | ||
| klass.getinfo(matlab_cmd, paths, use_mcr) | ||
| return klass._name | ||
|
|
||
|
|
@@ -178,17 +179,17 @@ def getinfo(klass, matlab_cmd=None, paths=None, use_mcr=None): | |
| returns None of path not found | ||
| """ | ||
|
|
||
| use_mcr = use_mcr or 'FORCE_SPMMCR' in os.environ | ||
| matlab_cmd = matlab_cmd or ((use_mcr and os.getenv('SPMMCRCMD')) | ||
| or os.getenv('MATLABCMD', 'matlab -nodesktop -nosplash')) | ||
|
|
||
| if klass._name and klass._path and klass._version and \ | ||
| klass._command == matlab_cmd and klass._paths == paths: | ||
|
There was a problem hiding this comment. This is related to your discussion with @effigies. Shouldn't we add some warnings if There was a problem hiding this comment. i'm not a big fan of warnings. almost no one reads it. There was a problem hiding this comment. well, you're probably right, but personally I sometimes read them if I feel hopeless... |
||
| return { | ||
| 'name': klass._name, | ||
| 'path': klass._path, | ||
| 'release': klass._version | ||
| } | ||
| mlab = MatlabCommand(matlab_cmd=matlab_cmd, resource_monitor=False) | ||
| mlab.inputs.mfile = False | ||
| if paths: | ||
|
|
@@ -214,6 +215,11 @@ def getinfo(klass, matlab_cmd=None, paths=None, use_mcr=None): | |
| # if no Matlab at all -- exception could be raised | ||
| # No Matlab -- no spm | ||
| logger.debug('%s', e) | ||
| klass._version = None | ||
| klass._path = None | ||
| klass._name = None | ||
| klass._command = matlab_cmd | ||
| klass._paths = paths | ||
| return None | ||
|
|
||
| out = sd._strip_header(out.runtime.stdout) | ||
|
|
@@ -225,6 +231,8 @@ def getinfo(klass, matlab_cmd=None, paths=None, use_mcr=None): | |
| klass._version = out_dict['release'] | ||
| klass._path = out_dict['path'] | ||
| klass._name = out_dict['name'] | ||
| klass._command = matlab_cmd | ||
| klass._paths = paths | ||
| return out_dict | ||
|
|
||
|
|
||
|
|
@@ -296,6 +304,10 @@ def set_mlab_paths(cls, matlab_cmd=None, paths=None, use_mcr=None): | |
| cls._matlab_cmd = matlab_cmd | ||
| cls._paths = paths | ||
| cls._use_mcr = use_mcr | ||
| info_dict = Info.getinfo( | ||
| matlab_cmd=matlab_cmd, | ||
| paths=paths, | ||
| use_mcr=use_mcr) | ||
|
|
||
| def _find_mlab_cmd_defaults(self): | ||
| # check if the user has set environment variables to enforce | ||
|
|
||
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.
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'm conceptually a little displeased with this memoization principle, but in practice I imagine nobody's using multiple MATLABs and SPMs within a single nipype script, so it's probably fine.
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.
actually i think this exactly handles the multiple matlab/spm scenario. let's say i have two different versions of spm. i can explicitly use Info to retrieve them (yes, it will loose the first memory on collecting the second one).
the situation, i think, boils down to this.
SPMCommand.set_mlab_pathsis a class method, so every instance derived will respect it. and i can adjust any given instance to change its command/path.Infoonly has class methods and we would somehow need to detect a change. this method decouples the change detection saying the only memory it has is when you pass the same parameters.an alternative was the previous augmented version to set the
Infoattributes everytimeset_mlab_pathsis called, retrieve that information and store it and always return that unless its changed again with get_info call.i don't feel strongly about either. just think that the latter seems more magical.
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.
Agreed, in the sense that this will be correct. The issue I was seeing with multiple versions I have is that you're running a MATLAB instance every time the cache is invalidated to get the information.
But it really doesn't matter. If people start complaining about slowdowns, we can consider keeping a memoizing dict keyed on a 3-tuple. This is probably fine, though.