Consolidate setting stream in creation of request file and variables list#380
Conversation
|
In the spirit of "lean" development, this just overwrites two hardcoded instances of the string I've put this variable into In future we are going to want to have multiple streams, which this doesn't deliver. One way to do this (in future) would be to create an interim |
NParsonsMO
added
standardise
| ROSE_TASK_APP = configure_standardise | ||
| START_YEAR = {{ START_YEAR }} | ||
| NUMBER_OF_YEARS = {{ NUMBER_OF_YEARS }} | ||
| STREAM_ID = "apm" |
There was a problem hiding this comment.
Hard-coding STREAM_ID here is not acceptable unless this is not meant to be configurable. Best practice (if it is configurable by end-user) is to have a template variable in rose-suite.conf (STREAM_ID="apm") and then in flow.cylc should access it like: STREAM_ID = {{ STREAM_ID }}. If this is a site constant then add it in site/metoffice.cylc.
There was a problem hiding this comment.
I thought that this PR was just to only hard code it in the one place (it had previously been two), while we awaited a decision on the shape of the future solution.
Not all variables will have the same stream, so I don't think it's user configurable, I think it's something where we will either need to create a lookup table, or create the request in two stages (CDDS has a guess stream function)..
I don't know which we're doing. Or if perhaps there is a different solution.
There was a problem hiding this comment.
@alistairsellar Does this meet the intended acceptance criteria? I think I focussed on "duplication",
But the line above the acceptance criteria implies that you had intended this to be a full solution to the problem of each variable potentially having a different stream.
There was a problem hiding this comment.
Hrrm, sorry, the issue description was quite ambiguous because it had that implementation point, which contradicted the point above it: "(In the future the required streams will be inferred from the variables to be extracted and the MIP table). I think that the implementation point was only relevant to the bit in brackets.
My understanding is that the acceptance criteria are right, so I've tidied up the rest to clarify. This issue should not make the stream configurable. We probably don't ever want this to be user configurable, since the future is to infer streams from variables.
mo-nikosbaltas
left a comment
mo-nikosbaltas
left a comment
There was a problem hiding this comment.
Following updates on the issue #213 of this PR, ACs are satisfied. Tests run successfully so good to go.
4 participants
Closes #213 .
PR creation checklist for the developer
<issue_number>above ☝️ has been replaced with the issue number.mainhas been selected as the base branch.<issue_number>_<short_description_of_feature>.good first issuelabel) have been added to the PR.Climate Model Evaluation Workflow (CMEW)project has been added to the PR.Definition of Done for the developer
docdirectory, including the Quick Start section; select one of the following):PR creation checklist for the reviewer
<issue_number>above ☝️ has been replaced with the issue number.mainhas been selected as the base branch.<issue_number>_<short_description_of_feature>.good first issuelabel) have been added to the PR.Climate Model Evaluation Workflow (CMEW)project has been added to the PR.Definition of Done for the reviewer
docdirectory, including the Quick Start section; select one of the following):Important
#<pull_request_number>: <pull_request_title>when writing the merge commit message for the pull request, so the pull request number is immediately visible on GitHub, regardless of the length of the pull request title.