Orca MM Runner

[MattBurn]

Closes Issues

Closes #185

Description

Adds OrcaMmRunner in opi.execution.orca_mm for executing supported orca_mm commands through opi.

The runner provides typed helpers for:

• orca_mm -convff
• orca_mm -splitff
• orca_mm -mergeff
• orca_mm -repeatff
• orca_mm -splitpdb
• orca_mm -mergepdb
• orca_mm -makeff
• orca_mm -getHDist

Each command-specific method validates its input files and arguments, assembles the appropriate command line, executes orca_mm, and verifies that the expected output files were generated.

The lower-level run_orca_mm() method builds on the updated BaseRunner.run() API introduced in #238. It supports stdout and stderr stream targets, returns a RunResult, and raises OrcaMmError by default when orca_mm exits with a nonzero return code. STDERR is captured when required so that it can be included in the error message while still being forwarded to any caller-provided stream targets.

---

Release Notes

Added

• Added OrcaMmRunner with typed helpers for supported orca_mm commands. (Orca MM Runner #224)
• Added OrcaBinary.ORCA_MM. (Orca MM Runner #224)
• Added OpiError, OpiExecutionError, and OrcaMmError. (Orca MM Runner #224)
• Added input-file validation and expected-output verification for orca_mm command helpers. (Orca MM Runner #224)

[timmyte]

@MattBurn
Thanks a lot for the PR. Thanks a very nice addition and improvement for OPI. Also thanks to directly include proper tests.

I would appreciate if you could overall add more comments and documentation to the code. Most patterns are very repetitive, so they would only a single documentation somewhere prominently on module level.

[MattBurn]

With the newly merged #235, I rebased this branch to allow the use of the new BaseRunner.run method. This helped to make the implementation of OrcaMmRunner simpler and removing the ambiguous behavior between silent=True and raise_on_error=True.

I have resolved most comments from the previous review, if you are not happy with any of the resolutions feel free to let me know. I have left some comments unresolved:

• Removing existing files
  • I have added the clarifying comments on why we need to remove the existing file so that we can verify the orca_mm command produced our expected output. I am still unsure of whether returning the expected output path if the file exists is the safest option and whether you have an opinion on the matter.
• Adding a file check
  • I added this as a staticmethod but could be moved to a module level helper function (stylistic choice I am fine with either)
• Docstring leading dash
  • I have been consistent throughout the docstrings regarding using the leading dash when referencing which orca_mm command the function will run. I do this because I am always referencing against the orca_mm binary rather than the OrcaMmRunner.run_orca_mm method which does not require the leading dash. All docstrings are consistent against referencing the underlying orca_mm command but I can see how that could lead to confusion regarding the use of the OrcaMmRunner.run_orca_mm function. I'd be happy updating this if you feel like it needs it.

Would I be able to request a new review to make sure I have properly resolved the comments from the previous review and to check through the integration with the new BaseRunner.run method. So far the test suite for OrcaMmRunner is quite light so if you feel like more testing is required then just let me know.

Hagen takes over

[nakul680]

Im wondering if this test should include a more specific field for stderr. Since the error is reported as:

f"orca_mm failed with the following error:\n{error.strip()}"

the test would pass regardless of what string value is set for stderr. A more specific error text would make for a better test

[nakul680]

It would be good to resolve the file path here before checking whether the file exists. That could be done either here or before this method is called , I would leave that decision up to you .