🚸 Add stderr to generator when streaming

[shnizzedy]

Is your feature request related to a problem? Please describe.
When streaming with spython, stdout is yielded to a generator, and if the subprocess's return_code is not 0, a CalledProcessError is raised. The contents of stderr is not passed in either case.

Describe the solution you'd like
This pull request adds the contents of stderr to stdout so the generator contains both in the same sequence as they would be presented if run outside of spython.

Describe alternatives you've considered
Alternatively, the contents of stderr could be included in the CalledProcessError to give the user details of the exception separate from stdout.

Test your PR locally, and provide the steps necessary to test for the reviewers.
I'm using this branch in the develop branch of cpac-python-package, specifically to wrap the "View crashfiles" series of commands into

cpac --platform singularity crash crash-file.pklz

The output of the wrapped command all goes to stderr, so without this change, the output stops just past

Logging messages will refer to the Singularity paths.

With this change, the stderr from the pickle is output to the terminal after the logging messages message.

Additional context
I was going to point this PR at develop (as requested in CONTRIBUTING), but no such branch currently exists. I'm happy to open a new PR to point at a new development branch if that is desired.

[vsoch]

Good call, there isn't a develop branch! Could you point me to where that's written so I can fix it up? You did exactly the right thing to PR directly to master.

[vsoch]

Nevermind you linked to the CONTRIBUTING - I'll take a look. Could you give me a Singularity Python command / example to show this in action? Ideally I'd ask for a test, but if you don't want to write one, I'm good with showing how to test it here.

[vsoch]

Okay I'm trying to test this out with a basic command. How do you intend for this to work, given CalledProcessError? For example:

from spython.main import Client
for line in Client.execute(image="docker://busybox", command=["which", "idontexist"], stream=True): 
    print(line) 

INFO:    Using cached SIF image

---------------------------------------------------------------------------
CalledProcessError                        Traceback (most recent call last)
<ipython-input-9-cfaebdeef320> in <module>
----> 1 for line in Client.execute(image="docker://busybox", command=["which", "idontexist"], stream=True):
      2     print(line)

If the error is only needed given that we trigger a CalledProcessError, would it not make sense to stream it only if that exception is caught?

[vsoch]

The issue with just added the stream to stdout is that the user might not want the error stream added there. If it's indeed the case that this addition is just to help/show the user the error given that the command doesn't work, I think the try/except would make more sense. Let me know your thoughts.

[vsoch]

okay here is a suggestion that will print stderr to the terminal given an error (and still result in the exception). First, here is a test script. It prints messages to output and error, and exits with an error.

$ cat test.sh 
#!/bin/bash
echo "This is fine."
>&2 echo "error 1"
>&2 echo "error 2"
exit 1

and here is how I updated your code, note that we are sending stderr to a pipe:

    process = subprocess.Popen(
        cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True
    )
    for line in iter(process.stdout.readline, ""):
        if not re.search(no_newline_regexp, line):
            yield line
    process.stdout.close()
    return_code = process.wait()
    if return_code:
        print(process.stderr.read())
        raise subprocess.CalledProcessError(return_code, cmd)

So now we can again test with the Client:

from spython.main import Client
for line in Client.execute(image="docker://busybox", command=["/bin/sh",
   ...:  "test.sh"], stream=True): 
   ...:     print(line) 
   ...:                                                                         
This is fine.

INFO:    Using cached SIF image
error 1
error 2

---------------------------------------------------------------------------
CalledProcessError                        Traceback (most recent call last)
<ipython-input-2-5d5f1b9e5c8a> in <module>
----> 1 for line in Client.execute(image="docker://busybox", command=["/bin/sh", "test.sh"], stream=True):
      2     print(line)
      3 

Notice about that we see the stderr (this is the desired output correct?) If we remove the return value of 1, then we don't see the error at all:

for line in Client.execute(image="docker://busybox", command=["/bin/sh","test.sh"], stream=True): 
   ...:     print(line) 
   ...:                                                                         
This is fine.

Is this the functionality that you desire?

[shnizzedy]

This is great! I forgot to mention I had also thought about suggesting a parameter of whether to include/print/pipe the contents of stderr, but I think your solution of including only given an exception. For my use case, I only care that the information is available, but I can imagine plenty of uses that might conditionally want to include that information.

@vsoch, based on your suggestions and code above, I updated my branch and added tests.

I wonder if it would be preferable to print the subprocess stderr to sys.stderr instead of printing to sys.stdout: https://github.com/shnizzedy/singularity-cli/pull/1

[vsoch]

This looks great! Is there any reason you'd want to print an error to stdout? I'm not sure what would be expected by users. In that both are streams, stderr is equally a first class citizen in terms of a place to put content. I think having the two nicely separates these two needs, actually.

I think we are mostly good to go! I haven't used the capsys fixture (but I suspect it's akin to a Capturing class) but I'm not familiar with docker_container - could you walk me through how that works (what exactly is at the first index?)

[vsoch]

And your emoji game is on point! It makes it very fun to review :)

[shnizzedy]

Is there any reason you'd want to print an error to stdout?

As opposed to printing the error to stderr? Not that I can think of!

I haven't used the capsys fixture (but I suspect it's akin to a Capturing class) but I'm not familiar with docker_container - could you walk me through how that works (what exactly is at the first index?)

capsys is a built-in fixture in pytest for "accessing captured output from a test function".

docker_container comes from

singularity-cli/spython/tests/conftest.py

Lines 27 to 30 in bffb29a

	@pytest.fixture(scope="session")
	def docker_container(tmp_path_factory):
	folder = tmp_path_factory.mktemp("docker-img")
	return folder, Client.pull("docker://busybox:1.30.1", pull_folder=str(folder))

which gives a tuple of (temporary directory, path to busybox:1.30.1 built-from-Docker-Hub Singularity image).

And your emoji game is on point! It makes it very fun to review :)

Thanks! I have the blog post that convinced me to start using them in commit messages bookmarked on my machine that I'm socially distant from and didn't find it in a quick search, but I've been using this guide lately.

[vsoch]

Gosh it really has been that long that I forgot my own functions! I was actually looking at pytest and there is a docker_container example so I think I got distracted.

This looks great! I've just updated the CHANGELOG and bumped the version, so when tests pass again we should be good to merge. I'll draft a release shortly after that.

[vsoch]

Thank you @shnizzedy ! https://pypi.org/project/spython/0.0.81 It was a pleasure.

[shnizzedy]

Likewise. Thanks @vsoch!