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

fixed absolute paths and tested batch scripts #21

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

kevindougherty-noaa
Copy link
Collaborator

This pull request is for issue #17.

Fixed absolute paths in the batch shell scripts.
Tested each batch script and addressed bugs (wrong variable and function names)

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of requested changes for this PR.

Also, this doesn't totally address @aerorahul's issue he opened. I think what he was saying is the sample test YAML files have hard coded paths, and perhaps the better approach is to use relative paths here. I guess this is more of an issue with my convinfo2yaml scripts? We should probably move to this repo/modify those scripts.

PyGSIdir=/scratch1/NCEPDEV/da/$LOGNAME/PyGSI
OUTDIR=/scratch1/NCEPDEV/da/$LOGNAME/PyGSI/
PyGSIdir=../
OUTDIR=../
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works if the scripts directory is the same directory you submit the job from. I think you want to use something like PyGSIdir=`dirname "$0"```. Note there are two extra because I can't figure out how to make a literal back tick in GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also outdir should be somewhere else by default, even if it's just $PyGSIdir/out (may need a mkdir -p $OUTDIR)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several issues with this script:

  1. [email protected] should be eliminated. A non-NOAA user should be able to run this.
  2. This script sources a modulefile meant for hera. We should eliminate that line or make it generic. What if there are no modules?
  3. Establish a Data/ directory and then all paths are relative to that. Relative paths should not be relative to current working directory, and having paths such as ../ are trip hazards.

PyGSIdir=/scratch1/NCEPDEV/da/$LOGNAME/PyGSI
OUTDIR=/scratch1/NCEPDEV/da/$LOGNAME/PyGSI/
PyGSIdir=../
OUTDIR=../
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above.

@CoryMartin-NOAA
Copy link
Contributor

CoryMartin-NOAA commented Jan 4, 2021

@aerorahul @kevindougherty-noaa I think what we need is a top-level wrapper� / setup script that can copy/extract the GSI diags, set up the Data directory, create the YAML, and do whatever else it needs before submission.

My only issue is how best to make the 'module' generic. I don't particularly like the way the global workflow does it, but perhaps that's the way to go? (check for a path and if it exists, source a module file, else do nothing?)

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to approve and merge this with the understanding that the batch shell scripts will be removed/replaced/fixed with a subsequent PR including a driver script.

@CoryMartin-NOAA CoryMartin-NOAA merged commit 3babeda into main Jan 5, 2021
@CoryMartin-NOAA CoryMartin-NOAA deleted the feature/fix_paths branch January 5, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants