ENH/FIX: shell command output options

[ZLLentz]

Overview

This PR was created to address a bug, but in the process it grew to become a minor rework of how we parameterize PyDMShellCommand widgets.

Bug Explainer

In the PyDMShellCommand widget, we've been using subprocess.PIPE for the stdout and stderr arguments to subprocess.Popen when we don't want these streams to show in the caller's terminal.

Unfortunately, this creates a problem. If the subprocess generates a certain amount of stdout or stderr, the pipe "fills up" so to speak, and then the subprocess is stuck waiting on being allowed to write more to stdout. The subprocess will then "freeze" until the calling process reads enough of the stdout buffer to allow the subprocess to finish writing data.

I'm not sure if this affects all flavors of subprocesses, but this definitely affects shell commands that open chatty Python processes.

Changes Included

Overall, this PR expands and clarifies our options for stdout and stderr output from subprocesses launched from PyDMShellCommand widgets. Previously, we parameterized this via the redirectCommandOutput property, but this property name has become confusing. This parameter still exists and works similarly to before, but now we can explicitly swap between output modes for both stdout and stderr.

The new default, HIDE, will use subprocess.DEVNULL instead of the previous default that used subprocess.PIPE, which avoids the bug above. The legacy behavior is still accessible via the STORE option, but the user will need to be mindful that they don't keep a long-running process with lots of outputs to avoid the issues explained above.

shell_command.py

• Add TermOutputMode enum for the new properties with options HIDE, SHOW, and STORE. This was originally created using the style in MNT: Add support to enums in both in PyQt5 and PySide6 #1145 at the time of this PR but this broke designer for me so I've put it back to the Q_ENUMS etc. style.
• Create new stdout and stderr properties that can be set to the various options from TermOutputMode. These are used to select what we pass in as the stdout and stderr options to subprocess.Popen
• Switch the redirectCommandOutput property to modify the stdout property for backwards compatibility.
• Include a check that the user isn't setting both redirectCommandOutput and stdout on the same widget, logging a warning if they do.

test_shell_command.py

• Incidental: update existing tests that intentionally trigger the UserWarning from the command -> commands deprecation to expect the user warning or avoid it as appropriate so that the warning doesn't bubble up to the pytest output. (This wasn't necessary but it was annoying me)
• In existing tests that expected to be able to communicate with the Popen instance, set stdout to STORE so that they can continue to do so.
• Add a test that verifies that the new options work as expected.
• Add a test that verifies that the old option still works and spawns warnings as expected.

[ZLLentz]

This breaks two pydm unit tests that intend to check the process attribute's stdout stream. I'm not immediately sure what to do about this. I'll try to figure something out.

[ZLLentz]

I thought about this just a little bit more, and fixing the tests is trivial, but maybe this leads into some more broad design questions:

• Are there any active users who rely on the subprocess.PIPE in their .py PyDM screens to pull stdout from subprocesses launched from these buttons and process them in custom code?
• In lieu of specific live examples, are there good reasons (aside from testing) to keep subprocess.PIPE as an option?
• Is there then value in refactoring this widget to have full control over what we use for stdout and stderr? I could imagine us being able to select None, PIPE, or DEVNULL on both stdout and stderr. With some simplified naming, this might even be more intuitive than the existing redirectCommandOutput boolean which I think has confused people in the past. This would also allow people to propagate stderr from the subprocess to the main process's terminal, which hasn't ever been an option.

I am looking for insight from the the core PyDM team. I'm happy to implement something here that is more future-thinking than the small change currently in the PR diff, which does fix my team's problem but doesn't consider the above questions.

[jbellister-slac]

So I'm not aware of any users who rely on the subprocess.PIPE functionality, maybe we'll see some response in slack.

If we did want to keep this functionality, I think your proposal of None/PIPE/DEVNULL sounds perfect for it. As you say that is much clearer than the redirectCommandOuptut boolean which is not clear what setting it to true will actually do without looking at the code.

In lieu of specific live examples, are there good reasons (aside from testing) to keep subprocess.PIPE as an option?

Only one I can think of is the obvious one of someone out there using it already. But I think I'd be fine erring on the side of just going with what you've done in this PR and if someone out there does still want subprocess.PIPE and let's us know later then we can take another pass at this with the solution you suggested.

[ZLLentz]

In that case, I'll check back in with slack on Monday to see if there's any response, and if not I'll fix the tests and move on for now.

edit: I'll implement the full option redesign if that ends up being the easiest way to fix the test suite anyway

[ZLLentz]

After ruminating on this over the winter shutdown, I decided that there's a lot of value in clarifying the various options here, so I'll go ahead and make a full stdout/stderr option set.

I'm thinking the properties will simply be called stdout and stderr and the enum options will be:
hide
show
store

The pre-existing pydm default is to store both with redirectCommandOutput = False.
The new default will be hide on both to avoid the bug the sparked this PR.
redirectCommandOutput, if set, will toggle stdout between hide and show to maintain some backwards compatibility.
There will be a warning if you set both redirectCommandOutput and stdout in the same widget, but the last property set will take precedence.

The existing tests in the test suite will then use stdout = store and will otherwise be unchanged. I can make a new test that verifies that hide and show work as intended.

Does this seem reasonable? Or would you prefer I hold off on this and do something differently/not at all?

[jbellister-slac]

Sounds great to me! That is both more functional and clearer than the current option, and handles backwards compatibility fine. And I think hide as the new default makes sense.

[ZLLentz]

Remaining work:

• I tried to use the enum style changes from MNT: make enums use python Enum class and updated Q_ENUM (no 's') #1146 but this creates some instability in designer so I might just change it back and let y'all figure that out.
• The windows builds fail on my new tests, presumably because subprocess output is handled slightly differently on windows. I need to look more closely.
• Python 3.8 can't built at all but I think this is unrelated to the PR here

[ZLLentz]

The tests pass windows now, I think the remaining failure is a conda/mamba networking fluke (edit: the networking issue went away upon re-run). I think this is ready for review.

I've updated the PR description with some verbose explanation of the changes.

[ZLLentz]

Note also that this adds a new enum without considering the other active enum PRs. If this goes in before those, the other PRs will need updating. If those go in first, this PR will need updating to match.

[nstelter-slac]

Note also that this adds a new enum without considering the other active enum PRs. If this goes in before those, the other PRs will need updating. If those go in first, this PR will need updating to match.

we found some issues with the new enum stuff, so we can have this patch go first and i'll update enum patch accordingly