Skip to content

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Dec 5, 2017

Previously, the code was not exiting with status 1 when a task failed.

Previously, the code was not exiting with status 1 when a task
failed.
@xylar xylar added the bug label Dec 5, 2017
@xylar xylar self-assigned this Dec 5, 2017
@xylar xylar requested a review from milenaveneziani December 5, 2017 22:12
@xylar
Copy link
Collaborator Author

xylar commented Dec 5, 2017

@milenaveneziani, can you let me know if this fixes the problem you mentioned here?
E3SM-Project/a-prime#24 (comment)

@xylar
Copy link
Collaborator Author

xylar commented Dec 5, 2017

If so, we can merge it as normal into develop, merge develop into master and create a new tag. Or we can merge just this commit into master and develop both, and create a tag off of master. For the former, we would probably make that v0.6.5 and the latter v0.6.1 (since more has been added in the former case than just a quick bug fix, but maybe not enough for a minor version change).

@milenaveneziani
Copy link
Collaborator

milenaveneziani commented Dec 6, 2017

@xylar (and pinging @sterlingbaldwin here as well): if I test this on the command line (making one task fail on purpose by adding a random line to one code) and then I type echo $?, I get the expected exit status different from 0. But if I do the same through a bash script, I get an exit status=0...
I tried to google this but couldn't find a solution.

Here is my simple bash script:

edison11:milena/global/.../MPAS-Analysis> cat test.bash
#!/bin/bash

./run_mpas_analysis.py config.beta3rc10.edison
echo '***************'
echo $?
echo '***************'
if [ $? -ne 0 ]; then
  echo
  echo "Failed some ocean/ice diagnostics tasks"
  exit 1
fi

do you have any idea?

@xylar
Copy link
Collaborator Author

xylar commented Dec 6, 2017

@milenaveneziani, so I think the problem with the above script is that $? is the exit status of the previous command, which includes the echo command. Since echo is returning 0, that's what you're seeing in each case. Here is a version that would work as you are expecting:

#!/bin/bash

./run_mpas_analysis.py config.beta3rc10.edison
status=$?
echo '***************'
echo $status
echo '***************'
if [ $status -ne 0 ]; then
  echo
  echo "Failed some ocean/ice diagnostics tasks"
  exit 1
fi

@milenaveneziani
Copy link
Collaborator

ok, that's embarassing. you are right @xylar.

I also tried to change the climo start and end years inappropriately, but in that case I don't get exit status=1. I suppose that's because that is part of the setup_and_check phase?

@xylar
Copy link
Collaborator Author

xylar commented Dec 6, 2017

I believe that's right. If you break things during the setup_and_check phase, it just doesn't run those particular tasks. If that's causing trouble in a-prime, we can rethink that approach. One option would be to have setup_and_check return True or False depending on if the task should run but raise and error if something fatal happened and stop the whole analysis.

@xylar
Copy link
Collaborator Author

xylar commented Dec 6, 2017

If you'd like to change that behavior, we should probably create a new issue for that and a separate PR to address it.

@milenaveneziani
Copy link
Collaborator

let's think about that a bit more. In the meantime, this is a great fix for the run phase.

@xylar
Copy link
Collaborator Author

xylar commented Dec 6, 2017

@milenaveneziani, do you want to merge just this commit into master or make a new tag that includes the recent python 3 work and such?

@xylar xylar merged commit 6c69798 into MPAS-Dev:develop Dec 6, 2017
@xylar xylar deleted the fix_serial_exit_status branch December 6, 2017 14:41
@milenaveneziani
Copy link
Collaborator

Since it's only the python 3 commit (and not for example the changes in #282), I would just make a v0.65 tag like you suggested above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants