-
Notifications
You must be signed in to change notification settings - Fork 1
Enable switching off CDDS extract #300
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
base: main
Are you sure you want to change the base?
Conversation
NParsonsMO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still working through the actual code changes, but have commented on some copyright stuff (and didn't want to lose this progress over lunch).
Not exactly but very close (Nikos changed that)
I don't feel qualified to say (Nikos: If you run the unit tests, they should complete successfully. Also, try all three cases (ACs) on your terminal with -O metoffice -O unittest, and should run for the different settings)
I've ticked this as I think it does
I've ticked this as I agree it didn't need to change
Nothing in doc has changed so I haven't ticked these |
NParsonsMO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AC1: I agree that when run with EXTRACT=true, as is default, the workflow runs as expected.
AC2: I cannot make this work. I have tried copying previous data (using HOUSEKEEPING=false) to my datadir, as I'm not sure what "the same filesystem" means but this seemed like a likely place for a scientist to have data, but I just get failures in the CDDS workflow at validate_extract_apm. I think there needs to be clearer guidance on how to get this working.
AC3: I agree that the CMEW workflow fails when EXTRACT=false and EXTRACT_DATA_PATH="". It would be nice if it complained earlier, which might be possible using the GUI? But it does seem to do as required if this is not possible.
I have also raised queries about:
a) the naming of variables, and suggest referring to "raw" data
b) the unit tests, which I think are now set to run themselves? But all the other unit tests in the repo have to be run explicitly, so I'm not sure this is required? It's entirely possible I have misunderstood this.
mo-nikosbaltas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All requested changes were made, including the renaming of EXTRACT_ to RAW_ in different files.
|
I appreciate that the instructions on #282 on how to run for AC2 were confusing and somehow incorrect as I was adding stuff as I was going along. I have updated these instructions and I would like you to review by making a run as instructed there. Just follow from the heading UPDATED INSTRUCTIONS for AC2. This will be part of your review. |
NParsonsMO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am approving on the grounds that the raw data variables are now named as I requested, and that I can get the scenario described in AC2 working, using the step-by-step instructions here: #282 (comment)
I have left a question about the HOUSEKEEPING option, but I believe that @ehogan was involved in discussions about this anyway, so I don't feel that's my call.
mo-nikosbaltas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed HOUSEKEEPING=true 25547ab
Closes #282.
PR creation checklist for the developer
<issue_number>above ☝️ been replaced with the issue number?mainbeen selected as the base branch?<issue_number>_<short_description_of_feature>?good first issuelabel) been added to the PR?Climate Model Evaluation Workflow (CMEW)project been added to the PR?Definition of Done for the developer
docdirectory) related to the change been updated appropriately, including the Quick Start section?PR creation checklist for the reviewer
<issue_number>above ☝️ been replaced with the issue number?mainbeen selected as the base branch?<issue_number>_<short_description_of_feature>?good first issuelabel) been added to the PR?Climate Model Evaluation Workflow (CMEW)project been added to the PR?Definition of Done for the reviewer
docdirectory) related to the change been updated appropriately, including the Quick Start section?