env_run: capture and log subprocess streams centrally

Previously `subprocess.run` inherited stdio, so each adapter call site
was on its own for surfacing what the subprocess actually printed —
nobody did, and failures left the user with just a non-zero return
code and no traceback.

Capture stdout and stderr in `run_in_conda_env` (`capture_output=True,
text=True`) and log centrally:

- non-zero return: `logger.warning` with script path, return code,
  stdout, and stderr.
- success: `logger.debug` with script path and the captured streams,
  for post-mortem when something downstream of the subprocess goes
  wrong.

The captured streams remain on the returned `CompletedProcess`, so
adapters that want to inspect them still can. Adapter-side
`if returncode: logger.warning(job-context...)` calls stay — they add
job/species context the helper doesn't have — but they no longer need
to also re-implement capture and stream rendering.

`--no-capture-output` for conda/mamba is still correct: it tells the
launcher to forward child stdio to its own stdio (which subprocess
then captures via the pipe), avoiding conda's internal buffering.
Micromamba already streams by default and rejects the flag.

Author: calvinp0

arc/job/env_run.py

@@ -105,9 +105,14 @@ def run_in_conda_env(
     """Run ``python script_path *script_args`` inside the env that owns
     ``python_executable``, isolated from ARC's process env.
 
-    Returns the :class:`subprocess.CompletedProcess`. ``check=True``
-    raises on non-zero exit. Args are passed as a list, so no shell
-    quoting concerns.
+    stdout and stderr are captured and logged centrally — debug on
+    success, warning (with both streams and the return code) on
+    non-zero exit — so call sites don't each re-implement capture and
+    error reporting. The captured streams are also exposed on the
+    returned :class:`subprocess.CompletedProcess` (``.stdout`` /
+    ``.stderr``) for callers that need to inspect them. ``check=True``
+    raises ``CalledProcessError`` on non-zero exit. Args are passed as
+    a list, so no shell quoting concerns.
     """
     env_prefix = env_prefix_from_python(python_executable)
     launcher, extra_flags = _detect_launcher()
@@ -118,4 +123,15 @@ def run_in_conda_env(
         *script_args,
     ]
     logger.debug("env-run: %s", " ".join(argv))
-    return subprocess.run(argv, check=check)
+    result = subprocess.run(argv, check=check, capture_output=True, text=True)
+    if result.returncode:
+        logger.warning(
+            "env-run: %s exited with %d\nstdout:\n%s\nstderr:\n%s",
+            script_path, result.returncode, result.stdout, result.stderr,
+        )
+    else:
+        logger.debug(
+            "env-run: %s exited 0\nstdout:\n%s\nstderr:\n%s",
+            script_path, result.stdout, result.stderr,
+        )
+    return result

arc/job/env_run_test.py

@@ -5,6 +5,7 @@
 This module contains unit tests of the arc.job.env_run module.
 """
 
+import subprocess
 import unittest
 from unittest.mock import patch
 
@@ -16,6 +17,12 @@
 )
 
 
+def _completed(returncode=0, stdout='', stderr=''):
+    """Build a CompletedProcess for mocking subprocess.run."""
+    return subprocess.CompletedProcess(args=[], returncode=returncode,
+                                       stdout=stdout, stderr=stderr)
+
+
 class TestEnvPrefixFromPython(unittest.TestCase):
     """env_prefix_from_python should derive the env prefix from any
     interpreter path, not assume a literal envs/ segment."""
@@ -121,7 +128,8 @@ class TestRunInCondaEnv(unittest.TestCase):
     def test_argv_uses_prefix_and_extra_flags(self):
         with patch('arc.job.env_run._detect_launcher',
                    return_value=('/opt/conda/bin/conda', ['--no-capture-output'])), \
-                patch('arc.job.env_run.subprocess.run') as mock_run:
+                patch('arc.job.env_run.subprocess.run',
+                      return_value=_completed()) as mock_run:
             run_in_conda_env(
                 '/opt/conda/envs/tst_env/bin/python',
                 '/path/to/script.py',
@@ -138,13 +146,18 @@ def test_argv_uses_prefix_and_extra_flags(self):
                 '--flag', 'value',
             ],
         )
+        # Streams must be captured so the helper can log them centrally.
+        kwargs = mock_run.call_args.kwargs
+        self.assertTrue(kwargs.get('capture_output'))
+        self.assertTrue(kwargs.get('text'))
         # No shell=True — args go through as a list.
-        self.assertNotIn('shell', mock_run.call_args.kwargs)
+        self.assertNotIn('shell', kwargs)
 
     def test_micromamba_omits_no_capture_flag(self):
         with patch('arc.job.env_run._detect_launcher',
                    return_value=('/usr/bin/micromamba', [])), \
-                patch('arc.job.env_run.subprocess.run') as mock_run:
+                patch('arc.job.env_run.subprocess.run',
+                      return_value=_completed()) as mock_run:
             run_in_conda_env(
                 '/scratch/envs/ts_gcn/bin/python',
                 '/path/to/gcn.py',
@@ -162,14 +175,48 @@ def test_micromamba_omits_no_capture_flag(self):
     def test_check_kwarg_passes_through(self):
         with patch('arc.job.env_run._detect_launcher',
                    return_value=('/opt/conda/bin/conda', ['--no-capture-output'])), \
-                patch('arc.job.env_run.subprocess.run') as mock_run:
+                patch('arc.job.env_run.subprocess.run',
+                      return_value=_completed()) as mock_run:
             run_in_conda_env(
                 '/opt/conda/envs/tst_env/bin/python',
                 '/path/to/script.py',
                 check=True,
             )
         self.assertTrue(mock_run.call_args.kwargs.get('check'))
 
+    def test_failure_logs_warning_with_captured_streams(self):
+        completed = _completed(returncode=2, stdout='partial output\n',
+                               stderr='Traceback...\nValueError: boom\n')
+        with patch('arc.job.env_run._detect_launcher',
+                   return_value=('/opt/conda/bin/conda', ['--no-capture-output'])), \
+                patch('arc.job.env_run.subprocess.run', return_value=completed), \
+                patch('arc.job.env_run.logger') as mock_logger:
+            result = run_in_conda_env(
+                '/opt/conda/envs/tst_env/bin/python',
+                '/path/to/script.py',
+            )
+        self.assertEqual(result.returncode, 2)
+        mock_logger.warning.assert_called_once()
+        # Render the warning to verify it carries the actual stderr contents.
+        fmt, *args = mock_logger.warning.call_args.args
+        rendered = fmt % tuple(args)
+        self.assertIn('ValueError: boom', rendered)
+        self.assertIn('partial output', rendered)
+        self.assertIn('/path/to/script.py', rendered)
+
+    def test_success_logs_debug_not_warning(self):
+        with patch('arc.job.env_run._detect_launcher',
+                   return_value=('/opt/conda/bin/conda', ['--no-capture-output'])), \
+                patch('arc.job.env_run.subprocess.run',
+                      return_value=_completed(stdout='ok\n')), \
+                patch('arc.job.env_run.logger') as mock_logger:
+            run_in_conda_env(
+                '/opt/conda/envs/tst_env/bin/python',
+                '/path/to/script.py',
+            )
+        mock_logger.warning.assert_not_called()
+        mock_logger.debug.assert_called()
+
 
 if __name__ == '__main__':
     unittest.main()