Capture-solver-log #525
Capture-solver-log #525
Conversation
|
|
|
@pchtsp could you check this please? |
|
can you update with master again? the tests were failing for another reason. thanks. |
|
Ah I see, python 2.7 does not support type annotations |
There was a problem hiding this comment.
Thanks, this indeed does seem to solve #502, as the solver output would be in the solver_output variable, though I haven't tried running with this branch. I've put some comments since I noticed some things that might cause problems for us if merged as-is.
| return_code = sub_process_runner.run(args) | ||
|
|
||
| if return_code != 0: | ||
| print(return_code) |
There was a problem hiding this comment.
Should remove this, otherwise this can break standardized logging
| if pipe: | ||
| pipe.close() | ||
|
|
||
| class SubProcessRunner: |
There was a problem hiding this comment.
I'm not the maintainer of the project so this is just an opinion, but to make testing easier, this class could be split out at a higher level and tested independently, rather than having the class declared inline.
| raise PulpSolverError( | ||
| "Pulp: Error while trying to execute, use msg=True for more details" | ||
| + self.path | ||
| + "return_code={}\noutput={}".format( |
There was a problem hiding this comment.
Exception would be hard to read with return code having no space before it, maybe newline here? Or remove this? I'd also suggest rewording the full exception, because the suggestion to use msg=True for more details would print to stdout but if you capture the output via other means, it will mislead developers thinking that msg=True will give more details than already provided.
Asyncronously capture output of the solver and save it as a string.
Allows for both "msg" and "logPath" to be enabled.
Change applied only to class COIN_CMD