Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unbound variable (RSTDIR_ATM) in forecast_det.sh (minor bug) #1140

Closed
DavidHuber-NOAA opened this issue Dec 1, 2022 · 4 comments · Fixed by #1179 or #1186
Closed

Unbound variable (RSTDIR_ATM) in forecast_det.sh (minor bug) #1140

DavidHuber-NOAA opened this issue Dec 1, 2022 · 4 comments · Fixed by #1179 or #1186
Assignees
Labels
bug Something isn't working

Comments

@DavidHuber-NOAA
Copy link
Contributor

Your bug may already be reported!
Please search on the Issue tracker before creating one.

Expected behavior

FV3_GFS_det should run without error when called during gdasfcst jobs. RSTDIR_ATM is only a required variable for gfsfcst jobs (set in forecast_predet.sh:212) and should not be referenced.

Current behavior

RSTDIR_ATM is referenced during both gdas and gfs fcst steps, resulting in an unbound error at forecast_det.sh:43 during gdasfcst. It appears that this unbound error only affects the find command within the command substitution and so flow continues with filecount=0, which does not affect on the remainder of the function. I'm not sure if this is the usual/constant behavior when set -u is toggled.

Machines affected

Found on Google Cloud.

To Reproduce

Run a gdasfcst job and check the resulting log.

Possible Implementation

Replace
filecount=$(find $RSTDIR_ATM -type f | wc -l)
with
[[ $CDUMP = "gfs" ]] && filecount=$(find $RSTDIR_ATM -type f | wc -l)
or
remove $CDUMP = "gfs" from the proceeding if and move the filecount assignment and the proceeding if into a new if [ $CDUMP = "gfs" ]; then block.

@DavidHuber-NOAA DavidHuber-NOAA added the bug Something isn't working label Dec 1, 2022
@lgannoaa
Copy link
Contributor

@DavidHuber-NOAA, It looks to me the description is point to a different file. Please update and confirm this issue. The file in question should be forecast_det.sh. Line number for the logic in question is 43.

@DavidHuber-NOAA
Copy link
Contributor Author

@lgannoaa You are correct that the file is forecast_det.sh and the line in question is 43. This is what is stated in the description under "Current behavior". I also mention that the variable is created in forecast_predet.sh. I felt this was relevant information as it is apparent in forecast_predet.sh that the variable should only be used during GDAS forecasts. Apologies if this caused confusion.

@lgannoaa
Copy link
Contributor

@DavidHuber-NOAA , I have a file ready for you to test. Please let me know if it looks good for you, so I can create PR for it.
hera:/scratch1/NCEPDEV/global/Lin.Gan/git/PR/issue-1140/ush/forecast_det.sh

@DavidHuber-NOAA
Copy link
Contributor Author

@lgannoaa the changes look good to me. Thanks!

@lgannoaa lgannoaa self-assigned this Dec 14, 2022
WalterKolczynski-NOAA pushed a commit that referenced this issue Dec 15, 2022
The forecast was attempting to check `RSTDIR_ATM` for restart files, but that variable is not defined for gdas, resulting in an unbound variable error. Because the error occurred in a subshell, the error had no impact on the execution, just an unwanted error message in the log. Now that directory is only checked when the `$CDUMP` is 'gfs'.

Also fixes some linter warnings.

Fixes #1140
WalterKolczynski-NOAA pushed a commit that referenced this issue Dec 18, 2022
Undoes the portion of PR #1179 that caused a new bug while attempting to fix #1140, without removing the linter fixes. Instead `/dev/null` is 'searched' if `${RSTDIR_ATM}` is not defined. That situation will always result in zero files found, ensuring a rerun is not triggered.

Fixes #1140 
Fixes #1185
Moots #1190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants