Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Keep stdout stderr #522

Merged
merged 4 commits into from
Mar 11, 2019
Merged

Keep stdout stderr #522

merged 4 commits into from
Mar 11, 2019

Conversation

lars-petter-hauge
Copy link
Contributor

Issue
Continues the work on equinor/ert#194

Does include commits from #520, but I'll rebase and fix everything.. The #520 will probably be merged first anyway.

Approach
First off, use communicate from Popen when using PIPE in order to collect and make sure that the program does not deadlock.

A bit unsure how the error should propagate upwards on this one, and who should be responsible for storing the result. I think I made the proper call on where to put it.

This isn't complete, using PIPE also means that the stdout and stderr isn't sent to terminal, I'll fix that before it's merged so this doesn't change current behaviour

@ghost ghost added the in progress label Feb 28, 2019
joakim-hove
joakim-hove reviewed Feb 28, 2019
View reviewed changes
python/res/job_queue/ert_script.py Outdated
""" @rtype: str """
return self.__stdout

@stderr.setter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having public read&write properties with names stdout and stderr is in my opinion a bit misleading, it is not stdout and stderras such you are manipulating here after all, but rather the string content which has been written to the respective streams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh, agreed.

Did a quick test on how it feels when using the existing defaultStackTrace in ErtScript. That might be the most "how the rest works", but I think ErtScript still should keep the results and have it available.

@lars-petter-hauge
Copy link
Contributor Author

Alright, a bit back and forth here.. I've renamed the variables to stdoutdata and stderrdata, the same names as the python documentation has and removed the setters, should be less misleading?

Added some tests as well.

@joakim-hove
Copy link
Contributor

should be less misleading?

I think so yes.

@lars-petter-hauge lars-petter-hauge mentioned this pull request Mar 1, 2019
Merged
@lars-petter-hauge lars-petter-hauge force-pushed the keep_stdout_err branch 4 times, most recently from 26c987f to 039d731 Compare March 5, 2019 15:38
@lars-petter-hauge
Copy link
Contributor Author

lars-petter-hauge commented Mar 5, 2019 •
edited
Loading

Did some final changes to this one.

  1. The stdout is now piped to local variable using the communicate function from the Popen object. The communicate will however wait until the command has completed and thus the stdout will be flushed completely after the process has completed.. So there's no "live" output from workflows anymore - this might be a problem.

  2. The defaultStackTrace didn't use the error argument at all, I've changed it around a bit, but I guess now it's likely to be not None, and thus it'll always be used.

  3. Instead of returning the stacktrace from the function call (and then having it printed by the caller), it's printed within ErtScript and returning None

@lars-petter-hauge lars-petter-hauge changed the title WIP: Keep stdout stderr Keep stdout stderr Mar 5, 2019
Lars Petter Øren Hauge added 3 commits March 5, 2019 16:54
Keep stdout and stderr when job is completed
d6058b0
Use current functionality for sys
e6ffe69 
sys.exc.traceback has been deprecated since 1.5
sys.info() is also the only one of these functions that are python 3
compatible
Expose job results from workflow_runner
1afaaec
ManInFez
ManInFez approved these changes Mar 11, 2019
View reviewed changes
Copy link
Contributor

@ManInFez ManInFez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@lars-petter-hauge lars-petter-hauge merged commit 1ebf1b1 into equinor:master Mar 11, 2019
@ghost ghost removed the in progress label Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joakim-hove joakim-hove joakim-hove left review comments

@ManInFez ManInFez ManInFez approved these changes

Assignees

@lars-petter-hauge lars-petter-hauge

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lars-petter-hauge @joakim-hove @ManInFez