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.
| ] | ||
|
|
||
|
|
||
| class OrcaMmException(Exception): |
There was a problem hiding this comment.
@haneug
If we start implementing custom exceptions, we should have base class OpiError from which all custom exceptions are derived.
There was a problem hiding this comment.
What path would you like to take? I can either change all OrcaMmExceptions into built in exception types or add an OpiError/OpiException if the latter do you prefer exception or error?
There was a problem hiding this comment.
I generally like the idea of adding OpiError. We could have a small implementation in opi/exceptions.py and derive OpiExecutionError from it and OrcaMmError would then be a subclass of that I guess. Would that be okay with you @timmyte ?
| if raise_on_error and path.exists() and (error := path.read_text()): | ||
| raise OrcaMmException(command, arguments, error) | ||
|
|
||
| def run_convff( |
There was a problem hiding this comment.
Please document on class level, that there is also a method per command of orca_mm as this is this a repeating design pattern.
| """ | ||
| Base class that facilitates the execution of ORCA binaries. | ||
| Makes sure that correct ORCA binary and MPI libraries are used. | ||
| This class is intended to be subclassed to execute an ORCA binary. |
There was a problem hiding this comment.
Not exactly. The class itself already allows running any ORCA binary, but for smoother a API it is helpful to subclass it.
Please reflect that in the documentation. Thanks
| """ | ||
| 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.
| @@ -0,0 +1,539 @@ | |||
| """ | |||
There was a problem hiding this comment.
Also, please give the module the same name as the binary it executes/wraps. Thanks.
There was a problem hiding this comment.
does this also apply to the sore orca runner or just to orca mm? What would you like the module to be called? orca_mm.py , mm_runner.py, orca_mm_runner.py?
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)