FIX: TypeError bug for PBS process communication in python3 #1987
Conversation
CommandLine already uses the default encoding to decode the stdout/stderr of the command, so take advantage of that functionality rather than re-implementing. First try was to implement decoding pattern seen elsewhere - then I changed my mind and used a CommandLine object from the interfaces.base module. First Attempt ------------- Decode the stderr using the default encoding * Fixes TypeError bug for PBS jobs running on python3.x * Fixes issue nipy#1959 Second Attempt -------------- Use the nipype CommandLine class instead of Popen Remove unused modules Ignore exceptions in qstat CommandLine call.
Codecov Report
@@ Coverage Diff @@
## master #1987 +/- ##
==========================================
+ Coverage 72.29% 72.29% +<.01%
==========================================
Files 1089 1089
Lines 55586 55584 -2
Branches 8010 8010
==========================================
- Hits 40184 40183 -1
+ Misses 14148 14147 -1
Partials 1254 1254
Continue to review full report at Codecov.
|
|
Additionally, I have noticed other places in the codebase that perhaps would suffer from the same bug. I could attempt fixes for these Popen calls (via a similar method) however I wouldn't have the capability to test the other batch submission systems. In particular I have noticed there there are possible similar problems in:
There are some other places, but I would be less confident adjusting those to use CommandLine. |
| _, e = proc.communicate() | ||
| result = CommandLine('qstat {}'.format(taskid), | ||
| environ=dict(os.environ), | ||
| terminal_output='allatonce', |
There was a problem hiding this comment.
I'm not sure 'allatonce' generates both stdout and stderr in the runtime object. Have you checked?
There was a problem hiding this comment.
allatonce doesn't stream but still separates stdout and stderr
| stderr=subprocess.PIPE) | ||
| _, e = proc.communicate() | ||
| result = CommandLine('qstat {}'.format(taskid), | ||
| environ=dict(os.environ), |
There was a problem hiding this comment.
I think you can remove this line if you want to use the default environment
|
@gjcooper - i'll merge this, but it would be good to clean up the other places where you spot this. |
When using python3 to call the PBS status command line script there is a TypeError crash (bytes-like object not str required). To fix this I have used the interfaces.base CommandLine class.
CommandLine already uses the default encoding to decode the
stdout/stderr of the command, so take advantage of that functionality
rather than re-implementing.
First try was to implement decoding pattern seen elsewhere - then I
changed my mind and used a CommandLine object from the interfaces.base
module. (It was already imported to the module anyway)
First Attempt
Decode the stderr using the default encoding
Second Attempt
Use the nipype CommandLine class instead of Popen
Remove unused modules
Ignore exceptions in qstat CommandLine call.