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

Add taskid and run deterministic and EnKF DA cycles. #524

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

hu5970
Copy link
Contributor

@hu5970 hu5970 commented Oct 11, 2024

Add taskid (jobid) to each task and run deterministic and EnKF DA cycles.

Tests:

  • WCOSS2
    • [x ] Cactus/Dogwood
  • RRFS_A:

Copy link
Contributor

@BenjaminBlake-NOAA BenjaminBlake-NOAA left a comment

Choose a reason for hiding this comment

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

@hu5970 Thanks for submitting this PR! I was planning to work on this soon. My initial PR was just for the non-DA tasks. I think some of the other J-jobs will need to be modified too, like JRRFS_ANALYSIS_GSI for example. Should those modifications also be included in this PR?

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-NOAA left a comment

Choose a reason for hiding this comment

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

One question about taskid/jobid - pretty sure Ben and I have discussed that previously, but not remembering the details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Question for you and @BenjaminBlake-NOAA

Since jobid is intended to be passed in from the "job card" - ecflow job in production, should we define it more like this?

export jobid=${jobid:-${taskid}}

Copy link
Contributor

Choose a reason for hiding this comment

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

@MatthewPyle-NOAA I agree that definition of $jobid is probably best, since we will replace $taskid with $job.$PBS_JOBID eventually.

@hu5970
Copy link
Contributor Author

hu5970 commented Oct 11, 2024

I added some of the jobid to the master branch for testing RRFS_A DA cycles. The system can run through for now but I certainly missed some JOBID. Could we merge this PR and then Ben can work on adding more JobID to other tasks?

@MatthewPyle-NOAA
Copy link
Contributor

@hu5970 That works for me. I opened an issue about it (#525) as a reminder.

Copy link
Contributor

@MatthewPyle-NOAA MatthewPyle-NOAA left a comment

Choose a reason for hiding this comment

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

Approving since this system runs, but with an issue (#525) about more uniformly applying a way to handle $jobid with rocoto-driven workflows, while also working well for ecflow-driven workflows.

@BenjaminBlake-NOAA
Copy link
Contributor

@hu5970 @MatthewPyle-NOAA Yes that sounds good to me. I will submit a PR for this early next week.

@MatthewPyle-NOAA MatthewPyle-NOAA merged commit f9c273b into NOAA-EMC:main Oct 11, 2024
2 checks passed
export INPUT_DATA=${DATAROOT}/${RUN}_forecast_${envir}_${cyc}/INPUT
fi
fi
export INPUT_DATA=${DATAROOT}/${taskid}/INPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect taskid here to be jobid, right?

Copy link
Contributor

@BenjaminBlake-NOAA BenjaminBlake-NOAA Oct 11, 2024

Choose a reason for hiding this comment

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

Yes it should be $jobid. I can plan to fix that with my upcoming PR where jobid=${jobid:-taskid}

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.

4 participants