Make extracted raw data location configurable by zmaalick · Pull Request #378 · MetOffice/CMEW

zmaalick · 2026-02-19T14:50:22Z

Closes #156.

PR creation checklist for the developer

The <issue_number> above ☝️ has been replaced with the issue number.
main has been selected as the base branch.
The feature branch name follows the format <issue_number>_<short_description_of_feature>.
The text of the PR title exactly matches with the text (not including the issue number) of the issue title.
Appropriate reviewers have been added to the PR (once it is ready for review).
The PR has been assigned to the developer(s).
The same labels as on the issue (except for the good first issue label) have been added to the PR.
The Climate Model Evaluation Workflow (CMEW) project has been added to the PR.
The appropriate milestone has been added to the PR.

Definition of Done for the developer

PR creation checklist for the reviewer

The <issue_number> above ☝️ has been replaced with the issue number.
main has been selected as the base branch.
The feature branch name follows the format <issue_number>_<short_description_of_feature>.
The text of the PR title exactly matches with the text (not including the issue number) of the issue title.
Appropriate reviewers have been added to the PR (once it is ready for review).
The PR has been assigned to the developer(s).
The same labels as on the issue (except for the good first issue label) have been added to the PR.
The Climate Model Evaluation Workflow (CMEW) project has been added to the PR.
The appropriate milestone has been added to the PR.

Definition of Done for the reviewer

Important

Remember to re-check the Definition of Done after making changes in response to a review.
The developer merges the PR.
Remember to use the format #<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.

NParsonsMO

I would like @ehogan to tell me whether this solution is OK, because it goes against what I thought was agreed in the technical meeting:

When I run the workflow specifying raw_data_dir="$SCRATCH/raw data", there are two folders that don't contain what I expect.

1 [user-specified]: MY_SCRATCH/raw_data
2 [workflow]: MY_HOME/cylc-run/CMEW/run3/share/data/cdds/raw_data

1 contains raw and processed data (though not restructured, I don't think).
It also contains lots of logs / empty directories and other "mess".

2 contains raw data (not processed or restructured).
It also contains empty directories and logs.

I expected 1 only to contain raw data, as the variable name implies.
I did not expect 2 to also contain raw data.

If we must keep all of it in the user-specified location, then the variable needs renaming as RAW_DATA_DIR is misleading.

CMEW/rose-suite.conf

ehogan · 2026-03-06T09:26:10Z

I would like @ehogan to tell me whether this solution is OK, because it goes against what I thought was agreed in the technical meeting:

When I run the workflow specifying raw_data_dir="$SCRATCH/raw data", there are two folders that don't contain what I expect.

1 [user-specified]: MY_SCRATCH/raw_data
2 [workflow]: MY_HOME/cylc-run/CMEW/run3/share/data/cdds/raw_data

1 contains raw and processed data (though not restructured, I don't think). It also contains lots of logs / empty directories and other "mess".

2 contains raw data (not processed or restructured). It also contains empty directories and logs.

I expected 1 only to contain raw data, as the variable name implies.
I did not expect 2 to also contain raw data.

Yes, 1 should only contain raw data.

2 will contain raw data. The workflow directory should look identical to a second workflow directory created by running CMEW with no raw_data_dir defined. It is my understanding that we agreed to copy or link the raw data such that it was available via the workflow directory, so that CDDS would act on that directory exactly as it would if raw_data_dir was not defined. Does that make sense?

NParsonsMO · 2026-03-06T09:31:29Z

Yes, 1 should only contain raw data.

2 will contain raw data. The workflow directory should look identical to a second workflow directory created by running CMEW with no raw_data_dir defined. It is my understanding that we agreed to copy or link the raw data such that it was available via the workflow directory, so that CDDS could act on that directory exactly as it would if raw_data_dir was not defined. Does that make sense?

I think that in a "normal" workflow [please correct me if I'm wrong] the raw data is cleaned up after CDDS runs, whereas here it was not (obviously not in the user-specified directory - we don't want that cleaned up) which is a difference within the workflow to how CMEW normally housekeeps after CDDS.

ehogan · 2026-03-06T09:33:33Z

I think that in a "normal" workflow [please correct me if I'm wrong] the raw data is cleaned up after CDDS runs, whereas here it was not (obviously not in the user-specified directory - we don't want that cleaned up - but it's a difference within the workflow to how CMEW normally housekeeps after CDDS).

The raw data in the workflow directory should still be cleaned up (CDDS should act on the workflow directory exactly as it would if raw_data_dir was not defined), but not in the user-specified directory, as you correctly pointed out 👍

zmaalick · 2026-03-06T11:38:13Z

after cdds extract in input dir (.share/data/..)
RAW_DATA_DIR: copy to raw data dir, (do not copy if RAW data dir is not empty)
RAW_DATA_DIR/suite_id/stream/data, if empty then copy data there and if not empty, print: log.err (dir was not empty)

zmaalick · 2026-03-09T11:58:42Z

The new solution:
Copy the input data (suite_id/apm/..) into the raw_data_dir (if provided) and job fails if raw_data_dir is not empty

Also update:

rose meta
branch with main

NParsonsMO

Minor:
The user-facing description / help for the new variable could be clearer.
More significant but I may be wrong:
I think that this is copying each dataset's folder within input individually, which may be nice as we could check whether each individual one exists and only copy it if it doesn't. But at the minute I think that you check whether the directory as a whole is empty (which would make sense if you were going to copy input as a whole), then copy each subdirectory individually, which seems like a wasted opportunity.
Also significant but I definitely am not sure about this:
The level of echoing etc. in the bash script made me think of the recently discussed PR where non of this was needed / desirable. Is this the same scenario?

NParsonsMO · 2026-03-10T09:27:43Z

CMEW/app/standardise_model_data/bin/standardise_model_data.sh

+# If RAW_DATA_DIR is configured, copy extracted raw data only when the target
+# directory is empty. If it is not empty, emit a log.err message and do not copy.
+if [[ -n "${RAW_DATA_DIR:-}" ]]; then
+    echo "[INFO] RAW_DATA_DIR is set to: ${RAW_DATA_DIR}"


Do we need all this stuff, or should it be a case of using set -xeu like in https://github.com/MetOffice/CMEW/pull/392/changes#r2895732243 ?

e5908c5

remove two echo statements, others are error logs, should be there

CMEW/meta/rose-meta.conf

zmaalick · 2026-03-10T09:53:55Z

Minor:
The user-facing description / help for the new variable could be clearer.

More significant but I may be wrong:
I think that this is copying each dataset's folder within input individually, which may be nice as we could check whether each individual one exists and only copy it if it doesn't. But at the minute I think that you check whether the directory as a whole is empty (which would make sense if you were going to copy input as a whole), then copy each subdirectory individually, which seems like a wasted opportunity.

Also significant but I definitely am not sure about this:
The level of echoing etc. in the bash script made me think of the recently discussed PR where non of this was needed / desirable. Is this the same scenario?

for point 2 above, I would suggest to scope it out as separate issue if it is intended. We can discuss it in technical meeting, if required, we can implement it in separate issue.

NParsonsMO · 2026-03-10T16:20:36Z

CMEW/app/standardise_model_data/bin/standardise_model_data.sh

Should this be here @ehogan? I know it hasn't changed.

I don't know whether GitHub is playing up, but I can't see which bit of the code this comment is for. Looking at 7ca1122, I infer that you are asking about pipefail? If so, the Developer Guide: Rose requirements explains where it should be used (it should not be used in bash scripts).

It was. I wondered if it meant the whole script had been changed at some point and now needed changing back (similarly to one last week). But that isn't actually part of this review.

NParsonsMO

Previous issues:

This has been clarified. The capital P on path presumably indicates that there was supposed to be a full stop, but I won't refuse based on this.

I do think this should be discussed, I don't think that I have authority to OK new issues. So one for @ehogan probably.
To the best of my ability to check, the bash script now follows the guidelines pointed out here:
#392 (comment)

add raw data path in config

6e7a799

zmaalick self-assigned this Feb 19, 2026

zmaalick added enhancement New feature or request technical debt Technical debt in CMEW standardise Anything related to CDDS labels Feb 19, 2026

zmaalick added this to the v0.2.0 (multiple model runs) milestone Feb 19, 2026

zmaalick added 3 commits February 19, 2026 14:55

remove unnecessary lines

d3bbb09

modify the solution

7f058ef

revert the previous changes

655b623

NParsonsMO self-requested a review March 4, 2026 11:49

NParsonsMO requested changes Mar 4, 2026

View reviewed changes

CMEW/rose-suite.conf Show resolved Hide resolved

zmaalick and others added 3 commits March 9, 2026 11:52

update the solution

f64b1f8

update precommit check

13af4d1

Merge branch 'main' into 156_raw_data_location_configurable

b818242

zmaalick requested a review from NParsonsMO March 9, 2026 11:58

NParsonsMO requested changes Mar 10, 2026

View reviewed changes

remove echo and update description

e5908c5

zmaalick requested a review from NParsonsMO March 10, 2026 09:49

lowercase the row

fb8b2e3

NParsonsMO reviewed Mar 10, 2026

View reviewed changes

zmaalick and others added 2 commits March 11, 2026 10:44

remove pipfail

7ca1122

Merge branch 'main' into 156_raw_data_location_configurable

95af99a

zmaalick requested a review from NParsonsMO March 12, 2026 10:10

NParsonsMO approved these changes Mar 12, 2026

View reviewed changes

Conversation

zmaalick commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR creation checklist for the developer

Definition of Done for the developer

PR creation checklist for the reviewer

Definition of Done for the reviewer

Uh oh!

NParsonsMO left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ehogan commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NParsonsMO commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehogan commented Mar 6, 2026

Uh oh!

zmaalick commented Mar 6, 2026

Uh oh!

zmaalick commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NParsonsMO left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NParsonsMO Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

zmaalick Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmaalick commented Mar 10, 2026

Uh oh!

NParsonsMO Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

zmaalick Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ehogan Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

NParsonsMO Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NParsonsMO left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zmaalick commented Feb 19, 2026 •

edited

Loading

NParsonsMO left a comment •

edited

Loading

ehogan commented Mar 6, 2026 •

edited

Loading

NParsonsMO commented Mar 6, 2026 •

edited

Loading

zmaalick commented Mar 9, 2026 •

edited

Loading

NParsonsMO left a comment •

edited

Loading

NParsonsMO Mar 12, 2026 •

edited

Loading

NParsonsMO left a comment •

edited

Loading