m3g / packmol

Packmol - Initial configurations for molecular dynamics simulations
http://m3g.github.io/packmol
MIT License
222 stars 51 forks source link

packmol returns exit code 0 even when errors #28

Closed rkingsbury closed 2 years ago

rkingsbury commented 2 years ago

I've encountered a number of cases in which the packmol binary returns an exit code of 0 even though the algorithm has clearly failed. For example, if packmol fails to find a solution within the required number of iterations:

  Maximum violation of the restraints:    2424667.4129541479
  ERROR: Packmol was unable to put the molecules
         in the desired regions even without
         considering distance tolerances.
         Probably there is something wrong with
         the constraints, since it seems that
         the molecules cannot satisfy them at
         at all.
         Please check the spatial constraints and
         try again.
# 679 water + 77 PEG + 81 cation + 163 anion
  >The maximum number of cycles (          80 ) was achieved.
   You may try increasing it with the nloop0 keyword, as in: nloop0 1000

or if there are errors in the input file:

  Reading input file... (Control-C aborts)
  ERROR: Number of molecules of type            1  set to less than 1
  Reading input file... (Control-C aborts)
  User defined GENCAN number of iterations:            0
  ERROR: Keyword not recognized: badarg

All of the above cases return an exit code of 0. This may be intentional, but intuitively, I would expect a nonzero exit code any time the output ends with an ERROR.

The reason this is important is that I'm developing scripts that call packmol automatically using python subprocess, and its error handling depends on detecting the exit code. If packmol always returns 0, there is no easy way for the calling script to recognize when it has failed.

lmiq commented 2 years ago

I think the easiest solution is to parse the error on the script. For example, this script does return an error code different from zero if the ERROR keyword was found:

#!/bin/bash
packmol_output=`packmol < $1`
if echo $packmol_output | grep -q "ERROR"; then
    exit -1
else
    exit 0
fi

Use with:

% ./pack.sh error.inp
% echo $?
255

of course something similar that parses the ERROR work in the output can be done directly with python.

rkingsbury commented 2 years ago

I think the easiest solution is to parse the error on the script. For example, this script does return an error code different from zero if the ERROR keyword was found:

#!/bin/bash
packmol_output=`packmol < $1`
if echo $packmol_output | grep -q "ERROR"; then
    exit -1
else
    exit 0
fi

Use with:

% ./pack.sh error.inp
% echo $?
255

of course something similar that parses the ERROR work in the output can be done directly with python.

Yes, thank you. I've implemented a workaround similar to this in my script for now. I feel this is inherently less reliable than exit code detection, though. Will packmol always place ERROR in stdout when it fails?

lmiq commented 2 years ago

It will in all errors that I handled manually (input type errors, etc). I will make that check ASAP, and update the package accordingly if there is any place where that does not happen.

I am also seeing the possibility of returning an error flag different from zero, but in terms of robustness it will be the same, because that will be thrown at the same points where now I just stop the program with no error flag.

rkingsbury commented 2 years ago

Makes sense. Thank you for checking on this @lmiq !

lmiq commented 2 years ago

Checked now, and "ERROR:" is everywhere where an improper input or output is found. In unpredictable cases you will get the program to crash and then the error flag will be different from zero.

afonari commented 1 year ago

Sorry for commenting on the closed case, but there are lots of infrastructure that acts on non-zero exit codes. @lmiq If you agree, I can create a pull request that adds the non-zero error codes to the stop places.

What do you think?

lmiq commented 1 year ago

I never worried much about that, so I don't really know what such a PR would be. But feel free to propose the changes. If they improve the usability of the package and do not introduce breaking changes or performance issues, it will be a great contribution.