Replace gmxapi `erroroutput` with `stderr` and `stdout`

diff --git a/docs/gmxapi/userguide/usage.rst b/docs/gmxapi/userguide/usage.rst
index cc9afd0cc5acd1b0f9dc323ec2bb8a3c7a9256d4..7cd08a764b748875803e90f5c5c99d8ddf41dfb0 100644 (file)
--- a/docs/gmxapi/userguide/usage.rst
+++ b/docs/gmxapi/userguide/usage.rst
@@ -145,7 +145,7 @@ For example, you might create a :command:`gmx solvate` operation as::
                                                       }
 
 To check the status or error output of a command line operation, refer to the
-``returncode`` and ``erroroutput`` outputs.
+``returncode`` and ``stderr`` outputs.
 To access the results from the output file arguments, use the command line flags
 as keys in the ``file`` dictionary output.
 

diff --git a/python_packaging/sample_restraint/tests/conftest.py b/python_packaging/sample_restraint/tests/conftest.py
index 37ef0767824a937fcd2a92c7ce5d3ff8526e0a8b..e439b3366ec92f2cc49a51ff09915f105703d67a 100644 (file)
--- a/python_packaging/sample_restraint/tests/conftest.py
+++ b/python_packaging/sample_restraint/tests/conftest.py
@@ -96,7 +96,7 @@ def spc_water_box(gmxcli, remove_tempdir):
         assert os.path.exists(topfile)
 
         if solvate.output.returncode.result() != 0:
-            logging.debug(solvate.output.erroroutput.result())
+            logging.debug(solvate.output.stderr.result())
             raise RuntimeError('solvate failed in spc_water_box testing fixture.')
 
         # Choose an exactly representable dt of 2^-9 ps (approximately 0.002)
@@ -132,7 +132,7 @@ def spc_water_box(gmxcli, remove_tempdir):
                                            output_files={'-o': tprfile})
         tprfilename = grompp.output.file['-o'].result()
         if grompp.output.returncode.result() != 0:
-            logging.debug(grompp.output.erroroutput.result())
+            logging.debug(grompp.output.stderr.result())
             raise RuntimeError('grompp failed in spc_water_box testing fixture.')
 
         # TODO: more inspection of grompp errors...

diff --git a/python_packaging/src/gmxapi/commandline.py b/python_packaging/src/gmxapi/commandline.py
index 0166dcebe13d56e1413aefabea6a3158c377f076..04269854925ccc7f6e5fa2f982975f64f416b432 100644 (file)
--- a/python_packaging/src/gmxapi/commandline.py
+++ b/python_packaging/src/gmxapi/commandline.py
@@ -60,7 +60,8 @@ logger.info('Importing {}'.format(__name__))
 #    * provides `output` publishing proxy to the inner function and
 #    * produce a result with attributes for
 #       * file: mapping of output flags to output filenames
-#       * erroroutput: text results in case of error
+#       * stdout: process STDOUT
+#       * stderr: porcess STDERR
 #       * returncode: integer return code of wrapped command
 #
 # Note that the existence of the 'file' output map is expressed here, but
@@ -70,7 +71,9 @@ logger.info('Importing {}'.format(__name__))
 #
 # TODO: Operation returns the output object when called with the shorter signature.
 #
-@gmx.function_wrapper(output={'erroroutput': str, 'returncode': int})
+@gmx.function_wrapper(output={'stdout': str,
+                              'stderr': str,
+                              'returncode': int})
 def cli(command: NDArray, shell: bool, output: OutputCollectionDescription, stdin: str = ''):
     """Execute a command line program in a subprocess.
 
@@ -117,23 +120,21 @@ def cli(command: NDArray, shell: bool, output: OutputCollectionDescription, stdi
             >>> my_filename = "somefilename"
             >>> result = cli(('exe', '--origin', 1.0, 2.0, 3.0, '-f', my_filename), shell=False)
             >>> assert hasattr(result, 'file')
-            >>> assert hasattr(result, 'erroroutput')
+            >>> assert hasattr(result, 'stdout')
+            >>> assert hasattr(result, 'stderr')
             >>> assert hasattr(result, 'returncode')
 
     Returns:
-        A data structure with attributes for each of the results `file`, `erroroutput`, and `returncode`
+        A data structure with attributes for each of the results `file`, `stdout`, `stderr`, and `returncode`
 
     Result object attributes:
         * `file`: the mapping of CLI flags to filename strings resulting from the `output` kwarg
-        * `erroroutput`: A string of error output (if any) if the process failed.
+        * `stdout`: A string mapping from process STDOUT.
+        * `stderr`: A string mapping from process STDERR; it will be the
+                    error output (if any) if the process failed.
         * `returncode`: return code of the subprocess.
 
     """
-    # Note: we could make provisions for stdio filehandles in a future version. E.g.
-    # * STDOUT is available if a consuming operation is bound to `output.stdout`.
-    # * STDERR is available if a consuming operation is bound to `output.stderr`.
-    # * Otherwise, STDOUT and/or STDERR is(are) closed when command is called.
-
     # In the operation implementation, we expect the `shell` parameter to be intercepted by the
     # wrapper and set to False.
     if shell:
@@ -154,7 +155,8 @@ def cli(command: NDArray, shell: bool, output: OutputCollectionDescription, stdi
     # TODO: (FR9) Can OS input/output filehandles be a responsibility of
     #  the code providing 'resources'?
 
-    erroroutput = ''
+    stdout = ''
+    stderr = ''
     logger.debug('executing subprocess')
     try:
         completed_process = subprocess.run(command,
@@ -162,20 +164,38 @@ def cli(command: NDArray, shell: bool, output: OutputCollectionDescription, stdi
                                            input=stdin,
                                            check=True,
                                            stdout=subprocess.PIPE,
-                                           stderr=subprocess.STDOUT,
+                                           stderr=subprocess.PIPE,
                                            encoding='utf-8',
                                            universal_newlines=True
                                            )
         returncode = completed_process.returncode
         # TODO: Resource management code should manage a safe data object for `output`.
-        for line in completed_process.stdout.split('\n'):
-            logger.debug(line)
+        logger.debug('STDOUT:')
+        if completed_process.stderr is not None:
+            for line in completed_process.stdout.split('\n'):
+                logger.debug(line)
+        else:
+            logger.debug('STDOUT is empty')
+        logger.debug('STDERR:')
+        if completed_process.stderr is not None:
+            for line in completed_process.stderr.split('\n'):
+                logger.debug(line)
+        else:
+            logger.debug('STDERR is empty')
+
+        stdout = completed_process.stdout
+        stderr = completed_process.stderr
+
     except subprocess.CalledProcessError as e:
-        logger.info("commandline operation had non-zero return status when calling {}".format(e.cmd))
-        erroroutput = e.output
+        logger.info("commandline operation had non-zero return status"
+                    "when calling {}".format(e.cmd))
+        stdout = e.stdout
+        stderr = e.stderr
         returncode = e.returncode
+
     # Publish outputs.
-    output.erroroutput = erroroutput
+    output.stdout = stdout
+    output.stderr = stderr
     output.returncode = returncode
 
 
@@ -253,7 +273,9 @@ def commandline_operation(executable=None,
         The output node of the resulting operation handle contains
 
         * ``file``: the mapping of CLI flags to filename strings resulting from the ``output_files`` kwarg
-        * ``erroroutput``: A string of error output (if any) if the process failed.
+        * ``stdout``: A string mapping from process STDOUT.
+        * ``stderr``: A string mapping from process STDERR; it will be the
+                      error output (if any) if the process failed.
         * ``returncode``: return code of the subprocess.
 
     """
@@ -284,15 +306,23 @@ def commandline_operation(executable=None,
     #
     # TODO: (FR4+) Characterize the `file` dictionary key type:
     #  explicitly sequences rather than maybe-string/maybe-sequence-of-strings
-    @gmx.function_wrapper(output={'erroroutput': str, 'returncode': int, 'file': dict})
-    def merged_ops(erroroutput: str = None, returncode: int = None, file: dict = None,
+    @gmx.function_wrapper(output={'stdout': str,
+                                  'stderr': str,
+                                  'returncode': int,
+                                  'file': dict})
+    def merged_ops(stdout: str = None,
+                   stderr: str = None,
+                   returncode: int = None,
+                   file: dict = None,
                    output: OutputCollectionDescription = None):
-        assert erroroutput is not None
+        assert stdout is not None
+        assert stderr is not None
         assert returncode is not None
         assert file is not None
         assert output is not None
         output.returncode = returncode
-        output.erroroutput = erroroutput
+        output.stdout = stdout
+        output.stderr = stderr
         if returncode == 0:
             output.file = file
         else:
@@ -328,7 +358,8 @@ def commandline_operation(executable=None,
     # TODO: ``label`` kwarg
     # TODO: input fingerprinting
     cli_result = cli(**cli_args)
-    merged_result = merged_ops(erroroutput=cli_result.output.erroroutput,
+    merged_result = merged_ops(stdout=cli_result.output.stdout,
+                               stderr=cli_result.output.stderr,
                                returncode=cli_result.output.returncode,
                                file=output_files,
                                **kwargs)

diff --git a/python_packaging/src/test/conftest.py b/python_packaging/src/test/conftest.py
index 15ca729861d833f02d66da6fe98471756a40843b..b80c21d4392ecc6044903236b1997e44a91f458f 100644 (file)
--- a/python_packaging/src/test/conftest.py
+++ b/python_packaging/src/test/conftest.py
@@ -94,7 +94,7 @@ def spc_water_box(gmxcli, remove_tempdir):
         assert os.path.exists(topfile)
 
         if solvate.output.returncode.result() != 0:
-            logging.debug(solvate.output.erroroutput.result())
+            logging.debug(solvate.output.stderr.result())
             raise RuntimeError('solvate failed in spc_water_box testing fixture.')
 
         # Choose an exactly representable dt of 2^-9 ps (approximately 0.002)
@@ -130,8 +130,7 @@ def spc_water_box(gmxcli, remove_tempdir):
                                            output_files={'-o': tprfile})
         tprfilename = grompp.output.file['-o'].result()
         if grompp.output.returncode.result() != 0:
-            # TODO: update for #3549
-            logging.debug(grompp.output.erroroutput.result())
+            logging.debug(grompp.output.stderr.result())
             raise RuntimeError('grompp failed in spc_water_box testing fixture.')
 
         # TODO: more inspection of grompp errors...

diff --git a/python_packaging/src/test/test_commandline.py b/python_packaging/src/test/test_commandline.py
index 94956b7704e1cb33ed39c6abbd2e67a4a54312a3..b5dd1838fe056390d9715f37a712e9c6b2a1aa59 100644 (file)
--- a/python_packaging/src/test/test_commandline.py
+++ b/python_packaging/src/test/test_commandline.py
@@ -60,15 +60,12 @@ class SimpleCliTestCase(unittest.TestCase):
         command = shutil.which('true')
         operation = commandline.cli(command=[command], shell=False)
 
-        # Note: 'stdout' and 'stderr' not mapped.
         # Note: getitem not implemented.
-        # assert not 'stdout' in operation.output
-        # assert not 'stderr' in operation.output
-        assert not hasattr(operation.output, 'stdout')
-        assert not hasattr(operation.output, 'stderr')
-
-        # Check for the attributes that we _do_ expect.
-        assert hasattr(operation.output, 'erroroutput')
+        # assert 'stdout' in operation.output
+        # assert 'stderr' in operation.output
+        assert hasattr(operation.output, 'stdout')
+        assert hasattr(operation.output, 'stderr')
+        assert not hasattr(operation.output, 'erroroutput')
         assert hasattr(operation.output, 'returncode')
 
         operation.run()
@@ -118,14 +115,13 @@ class CommandLineOperationSimpleTestCase(unittest.TestCase):
 
     def test_true(self):
         operation = commandline.commandline_operation(executable='true')
-        # Note: 'stdout' and 'stderr' not mapped.
         # Note: getitem not implemented.
-        # assert not 'stdout' in operation.output
-        # assert not 'stderr' in operation.output
-        assert not hasattr(operation.output, 'stdout')
-        assert not hasattr(operation.output, 'stderr')
+        # assert 'stdout' in operation.output
+        # assert 'stderr' in operation.output
+        assert not hasattr(operation.output, 'erroroutput')
         assert hasattr(operation.output, 'file')
-        assert hasattr(operation.output, 'erroroutput')
+        assert hasattr(operation.output, 'stdout')
+        assert hasattr(operation.output, 'stderr')
         assert hasattr(operation.output, 'returncode')
         assert operation.output.returncode.result() == 0