search

ufs-community / ufs-weather-model

UFS Weather Model
Other
139 stars 248 forks source link

[BUG]: rt.sh reuses old executables which can cause confusion if a more recent build fails #228

Closed rsdunlapiv closed 3 years ago

rsdunlapiv commented 4 years ago

Describe the bug The rt.sh script will reuse an old exectuable (e.g., fcst_1.exe) if the current build fails. This leads to confusion because the user thinks they are using a new build, but if the build fails, it may copy in an old executable and run the regression tests using that. You cannot really tell that it is using an old executable unless you examine compile_1.log and see the failure.

To Reproduce (1) Run rt.sh for a single test (2) Change the code to create a compile-time error (3) Run rt.sh again (4) The regression tests will run the executable created in (1)

Expected behavior If the compile step fails, the rest of the regression tests should not run and you should get a clear error message.

Additional context This probably affects ufs-s2s-model and ufs-weather-model

DeniseWorthen commented 4 years ago

Do we believe this is an issue which is still present when using ufs-weather rt.sh? If yes, I will move the issue to the weather.

MinsukJi-NOAA commented 4 years ago

I believe this is still an issue, and we can address it in the weather repo.

BrianCurtis-NOAA commented 3 years ago

Has this since been addressed, or does it still need work?

DeniseWorthen commented 3 years ago

I think this is related to a comment I made on a recent PR where I asked if not removing the fv3_X.exe from when the RT fails was a feature or a bug here. Dom had said that he thought it was actually a useful feature because it allows you to re-use the executable for further testing. So I'm not sure it is a bug.

BrianCurtis-NOAA commented 3 years ago

@DeniseWorthen so would you say we can close this issue?

DeniseWorthen commented 3 years ago

@BrianCurtis-NOAA I thought about this at the time of the conversation I referenced. If the RT fails, I can understand why the fv3_X.exe are retained. In the case though where second rt.sh is executed in the same directory, then I do think the intention is that is always starts "clean", correct?

BrianCurtis-NOAA commented 3 years ago

Great point, i was trying to wrap my head around it a bit. I agree with enhancement!

DusanJovic-NOAA commented 3 years ago

If regression testing is successful all executables will be removed: See:

https://github.com/ufs-community/ufs-weather-model/blob/9004b5e7a6808aeca8591446e568f15141e0c70d/tests/rt.sh#L777

In all other cases nothing will be removed.

DeniseWorthen commented 3 years ago

Ah, yes my mistake. That line is currently present.

MinsukJi-NOAA commented 3 years ago

I am not able to reproduce this error for the following two cases:

1) Without using ecflow, and rt.conf consisting of a single COMPILE and a single RUN. 2) Using ecflow, and rt.conf consisting of COMPILE, RUN, COMPILE, RUN.

During the first run, if rt.sh successfully completes both COMPILE and RUN, the exe file will be automatically deleted as Dusan shows above. So, no issue here.

During the first run, if rt.sh completes COMPILE but fails the RUN, the exe file remains. During the second run, if COMPILE fails, rt.sh exits instead of proceeding to RUN using the exe remaining from the first run.

DeniseWorthen commented 3 years ago

Given the evolution of the build/run system since this issue was created, it is not clear what the status or reproducibility of the reported error is. I will close this issue.