-
Notifications
You must be signed in to change notification settings - Fork 532
FIX: Set default result in DistributedPluginBase._clean_queue #2596
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
Conversation
Please remove the unrelated auto test. That is being added elsewhere, and will currently cause this PR to fail. |
Also, I think a cleaner solution would be to set this default in nipype/nipype/pipeline/plugins/base.py Lines 220 to 227 in 9eaa2a3
I would suggest adding at line 225: if result is None:
result={'result': None,
'traceback': '\n'.join(format_exception(*sys.exc_info()))} |
@achetverikov Are you going to submit an updated PR? I'd like to get all fixes in today for the next release, and this seems like a quick one. |
yes, wait a sec, I just did a reset on my fork and apparently that closed the PR as well |
check the updated commit |
Codecov Report
@@ Coverage Diff @@
## master #2596 +/- ##
=========================================
- Coverage 67.62% 67.6% -0.02%
=========================================
Files 336 336
Lines 42711 42713 +2
Branches 5278 5279 +1
=========================================
- Hits 28883 28877 -6
- Misses 13151 13158 +7
- Partials 677 678 +1
Continue to review full report at Codecov.
|
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.
Great! Thanks. Just a couple style issues and I think this will be good to go.
nipype/pipeline/plugins/base.py
Outdated
'result': None, | ||
'traceback': '\n'.join(format_exception(*sys.exc_info())) | ||
})) | ||
graph)) |
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.
Maybe let's pull all this back onto one line:
self._clean_queue(jobid, graph)
nipype/pipeline/plugins/base.py
Outdated
@@ -222,6 +218,9 @@ def _clean_queue(self, jobid, graph, result=None): | |||
|
|||
if self._status_callback: | |||
self._status_callback(self.procs[jobid], 'exception') | |||
if result is None: | |||
result={'result': None, | |||
'traceback': '\n'.join(format_exception(*sys.exc_info()))} |
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 like you're using tabs, not spaces. Do you think you could update this to use 4-space indentation? Also, there should be spaces around the =
, and 'traceback'
should start on the same column as 'result'
.
(Sorry if this is a pain in your editor.)
done, I think |
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.
Awesome. Thanks for this.
you're doing great work guys, I'm glad to be of help |
Fixing this: #2570
Changes proposed in this pull request