Move shell out commands into separate library

[Pierre-siddall]

PR Summary

Code Reviewer: @ericaneininger

This PR aims to unify shell out commands across MOCI so that they all have the same semantics. The changes involved include adding a separate library to handle shell operations and replacing current shell operations with a newly implemented exec_subprocess function.

closes #12

Code Quality Checklist

(Some checks are automatically carried out via the CI pipeline)

• I have performed a self-review of my own code
• My code follows the project's style guidelines
• Comments have been included that aid undertanding and enhance the
  readability of the code
• My changes generate no new warnings

Testing

• I have tested this change locally, using the Moci rose-stem suite
• If any tests fail (rose-stem or CI) the reason is understood and
  acceptable (eg. kgo changes)
• I have added tests to cover new functionality as appropriate (eg. system
  tests, unit tests, etc.)

Security Considerations

• I have reviewed my changes for potential security issues
• Sensitive data is properly handled (if applicable)
• Authentication and authorisation are properly implemented (if applicable)

Performance Impact

• Performance of the code has been considered and, if applicable, suitable
  performance measurements have been conducted

AI Assistance and Attribution

• Some of the content of this change has been produced with the assistance
  of Generative AI tool name (e.g., Met Office Github Copilot Enterprise,
  Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the
  Simulation Systems AI policy
  (including attribution labels)

Documentation

• Where appropriate I have updated documentation related to this change and
  confirmed that it builds correctly

Code Review

• All dependencies have been resolved
• Related Issues have been properly linked and addressed
• CLA compliance has been confirmed
• Code quality standards have been met
• Tests are adequate and have passed
• Documentation is complete and accurate
• Security considerations have been addressed
• Performance impact is acceptable

[ericaneininger]

This is a good start.

General Comments:

• I would like to see this new module made available via a python Package implementation - for example a new package directory at top level -

   moci /
        |-> moci_utils /   (or moci_lib?)
                       |-> __init__.py
                       |-> shellout.py
                       |-> tests /

The idea would be to install this package along with the postproc or drivers code and import the required functions via from moci_utils import shellout. This gives us a clearer idea of where the methods are being imported form - especially useful as "shellout" and "timer" and "utils" are potentially common terms.

• All new code should be accompanied by unit tests. unittest is the package predominantly used in MOCI at the moment, but a more modern alternative would be acceptable.
• I suggest, particularly in the PostProc app case, that for the time being we do a proof of concept - without removing utils.exec_subproc entirely for the time being and just place a "deprecated in favour of moci_utils.shellout" tag in the docstring. Choose one file and replace the calls to utils.exec_subproc in there. Make sure that rose-stem still runs. We will open further issues to replace all calls once we have proof of concept.

[ericaneininger]

A couple of things to note here:

• Copyright notice required. Please use the appropriate (Python) Momentum template:
  https://github.com/MetOffice/Momentum/blob/main/docs/COPYRIGHT_TEMPLATE
• Unit tests required for this code. MOCI codebase test coverage currently uses unittest, mainly due to the age of the code. pytest may also suitable.
• Don't use timer here - this is PostProc functionality - leave it to the PostProc application to run any timing deemed necessary. As an aside - moving timer.py into the new mocilib package is a good idea.
• I think you've made a good decision to use subprocess.run() here, to replace the driver use of popen() and PP use of check_output(). However, a couple of things to note about the implementation:
  • L26 - I'm not convinced subprocess.stdin=True KW argument is required here. I believe that it is implied in internal calls to subprocess.communicate(), which we are not calling directly.
  • L29 - Hard-wiring timeout=10 is going to be a problem of for shell-outs which take minutes rather than seconds to complete - such as where it is required for means creation and compression of NetCDF files in PostProc. Perhaps an additional keyword argument to the routine at L9 would be useful - timeout=None?
  • L30 - Consider the use of the KW argument run(check=True) - I cannot think of a reason why we might NOT want to check the output? This saves the if statement at L35, since a subprocess.CalledProcessError exception would be raised if rcode != 0
  • L37 - The run(check_output) argument was added at Python3.7. This renders the version check here redundant. Policy, going forward, is not to support Python2 anyway. With this in mind - we may need to enforce Python3.7+ at package top level.
  • L40 - Consider catching the exception subprocess.TimeoutExpired
  • L43 - What is the use-case for the OSError exception being raised? Is this a copy and paste from PostProc, because if so, I'm not sure it is relevant for this implementation. Please use unit tests to ensure all code is executable as designed.
  • L47 - Please ensure that the return value types are (int, str) in all cases - In normal execution, output is currently an instance of subprocess.completedprocess. At L38, we need output = output.stdout.decode(). Possibly rename the return value output to avoid confusion.

[ericaneininger]

Here I think that where you have replaced common.exec_subproc_timeout(du_command, timeout) with shellout._exec_subprocess(du_command, timeout), what you have actually done is set keyword argument verbose=timeout. verbose is currently the only KW arg available currently.
Unit tests for this would make this error clear.

[ericaneininger]

I believe you are setting verbose=timeout here - not quite what you meant?

[ericaneininger]

If you have definitely replace all calls to these functions from drivers code, then by all means remove these methods. rose-stem will be required to check most of these. If you have only replaced a subset then please leave these here, with "Method deprecated in favour of moci_utils.shellout" added to the docstring.