search

OpenFreeEnergy / gufe

grand unified free energy by OpenFE
https://gufe.readthedocs.io
MIT License
28 stars 8 forks source link

Always raise, don't swallow exception, if exception is `KeyboardInterrupt` in `ProtocolUnit.execute` #215

Closed dotsdl closed 1 year ago

dotsdl commented 1 year ago

This PR is an attempt to address openforcefield/alchemiscale#132.

@mikemhenry raised a good point that for the case of a KeyboardInterrupt, even if raise_error = False for ProtocolUnit.execute, we probably always want to raise.

Is there any downside to this?

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2028a63) 99.12% compared to head (9146ea8) 99.12%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #215 +/- ## ======================================= Coverage 99.12% 99.12% ======================================= Files 36 36 Lines 1829 1831 +2 ======================================= + Hits 1813 1815 +2 Misses 16 16 ``` | [Files Changed](https://app.codecov.io/gh/OpenFreeEnergy/gufe/pull/215?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) | Coverage Δ | | |---|---|---| | [gufe/protocols/protocolunit.py](https://app.codecov.io/gh/OpenFreeEnergy/gufe/pull/215?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy#diff-Z3VmZS9wcm90b2NvbHMvcHJvdG9jb2x1bml0LnB5) | `97.24% <100.00%> (+0.03%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dwhswenson commented 1 year ago

My only concern would be if INT is being used elsewhere, e.g., by a queueing system, in which case we'd want to catch it and terminate cleanly. (Which also might be an option here, anyway.)

The queueing systems I know send TERM or USR2, followed by a KILL; I'm not aware of one using INT, but I don't claim to know them all!

If we ever handle mid-unit restarts, we'll need to handle KeyboardInterrupt with a little more logic, but that's easily added. Otherwise, seems reasonable -- either needs a test or exclude from coverage.

dotsdl commented 1 year ago

@dwhswenson thanks for this! I've added an explicit test for the new behavior.

As for the concern you raised about queueing systems, I think you're right that SIGINT generally isn't used for terminating jobs, so I think we're still good here.