Orca MM Runner#224
Conversation
timmyte
left a comment
timmyte
left a comment
There was a problem hiding this comment.
@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.
| """ | ||
| expected_output = structure_file.with_suffix(self._orca_ff_suffix) | ||
|
|
||
| arguments = [str(structure_file)] |
There was a problem hiding this comment.
Please add some more comments to this routine, to point out the individual steps and preparations done here, like,
- generating files
- assembling call
- performing call
| @@ -0,0 +1,50 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
Please add a module level description what is tested in this module and also list the individual tests.
| from opi.execution.mm import OrcaMmException, OrcaMmRunner | ||
|
|
||
|
|
||
| def _set_fake_orca_path(self, orca_path: Path | None = None) -> None: |
There was a problem hiding this comment.
Please add docstrings.
Mention why these fake are necessary as this is not trivial.
|
|
||
| @pytest.mark.unit | ||
| def test_run_orca_mm_handles_deleted_empty_stderr(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setattr(OrcaMmRunner, "set_orca_path", _set_fake_orca_path) |
There was a problem hiding this comment.
Didn't know Pytest has MonkeyPatch. Very sophisticated. Thanks!!
|
|
||
| runner = OrcaMmRunner() | ||
|
|
||
| def fake_run(binary, args, /, *, stderr=None, silent=True, timeout=-1): |
There was a problem hiding this comment.
Again, please add docstring. Why fake execution. Please describe your design. Could also do this on module level, as the pattern is the same.
Co-authored-by: Tim Tetenberg <123412573+timmyte@users.noreply.github.com>
Co-authored-by: Tim Tetenberg <123412573+timmyte@users.noreply.github.com>
Co-authored-by: Tim Tetenberg <123412573+timmyte@users.noreply.github.com>
3 participants
Closes Issues
Closes #185
Description
Extracts shared ORCA execution logic from
Runnerinto a newBaseRunnerand addsOrcaMmRunnerinopi.execution.mmfor runningorca_mm.OrcaMmRunneradds typed helpers for supportedorca_mmsubcommands, validates expected output files, and raises a dedicatedOrcaMmExceptionwhenorca_mmreports errors on stderr. This also extendsOrcaBinarywithORCA_MMand adds unit coverage for stderr handling, including the empty/deleted temporary stderr-file case.Release Notes
Added
BaseRunnerto share ORCA binary resolution, environment setup, version checks, and subprocess execution across runner implementations. (Orca MM Runner #224)OrcaMmRunnerwith helpers fororca_mmcommands such asconvff,splitff,mergeff,repeatff,splitpdb,mergepdb,makeff, andgetHDist. (Orca MM Runner #224)OrcaBinary.ORCA_MMand unit tests coveringorca_mmstderr handling. (Orca MM Runner #224)Changed
Runnerto inherit fromBaseRunner, keepingorca,orca_plot, andorca_2jsonbehavior while moving shared execution logic into the base class. (Orca MM Runner #224)