Skip to content

Add check for 0-byte files to cpreq - #7

Open
RussellManser-NCO wants to merge 7 commits into
NCO-HPC:v2.0.15from
RussellManser-NCO:cpreq_zero_byte
Open

Add check for 0-byte files to cpreq#7
RussellManser-NCO wants to merge 7 commits into
NCO-HPC:v2.0.15from
RussellManser-NCO:cpreq_zero_byte

Conversation

@RussellManser-NCO

@RussellManser-NCO RussellManser-NCO commented Oct 31, 2025

Copy link
Copy Markdown

Summary

This PR adds a check for 0-byte files in cpreq and an option to skip the 0-byte check. By default cpreq will now raise an error when one or more of its arguments are size 0.

Notes

  • This PR also adds: a) a full test suite for cpreq that covers many use cases and b) patched versions of err_exit and err_chk designed specifically for use in tests.
  • The tests directory is not allowed under the current implementation standards. I believe it should be included here to demonstrate that the current solution works and to make testing easier in the future. See [New Standard]: Add directory tests/ for unit tests nws-hpc-standards#17 for further discussion.

Todo

  • Assess and document which production packages expect to copy 0-byte files under normal conditions

Complete

  • Remove hard-coded paths from tests/test_cpreq.sh
  • Implement or remove teardown function in test/test_cpreq.sh
  • Specify failure conditions for test cases which are expected to fail
  • Add modifications to handle -t and -T options for cp
  • Fix bug for single-options cp cases
  • Add tests for long-form options
  • Test against packages on WCOSS
  • Test replacing cpreq with cp against packages on WCOSS
  • Test in para for 1 week (started on 11/07/2025 ~1644Z)

@RussellManser-NCO

RussellManser-NCO commented Nov 3, 2025

Copy link
Copy Markdown
Author

I found a couple of bugs that require fixing:

  • cp options are not handled correctly when the -z option is omitted. I am working on a fix and tests for single and multiple options
  • test_with_multiple_cp_flags() is a poorly constructed test because it includes the -i option for interactive copying

@StevenEarle-NCO

Copy link
Copy Markdown

Along these lines.... we do need to make sure cpreq can be replaced with cp in our scripts. cpreq isn't on other systems so people will alias cpreq to cp, and it still needs to just work like a normal cp command would. Can you verify that as you go through this work?

@RussellManser-NCO

Copy link
Copy Markdown
Author

Along these lines.... we do need to make sure cpreq can be replaced with cp in our scripts. cpreq isn't on other systems so people will alias cpreq to cp, and it still needs to just work like a normal cp command would. Can you verify that as you go through this work?

Yes, the tests I included so far aim to demonstrate and verify that cpreq works just like cp. As I noted I found that some further development and tests are needed to reach that goal.

@RussellManser-NCO

Copy link
Copy Markdown
Author

Testing cpreq against WCOSS packages

Description

Targeted packages/jobs

  • epoch.v1.0.7: calls cpreq from a python script
    • jepoch.ecf
  • ecmwf_mos.v1.2.3: uses long-form options
    • jecmwf_mos_prep
    • jecmwf_mos_prep23
    • jecmwf_mos_ext_prep23
    • jecmwf_mos_ext_prep
    • jecmwf_mos_stations_ens_fcst

Testing process
Performed tests using cpreq in this PR by following these steps:

  1. Setup package in test space
  2. Copy cpreq to ush/
  3. In the ecf script before calling the J-job, add
export PATH=${HOME<model>}/ush/cpreq:${PATH}
echo "cpreq is $( which cpreq )"
  1. Run job, check output files to confirm correct version of cpreq was used and that job completed successfully

Performed tests to "alias" cpreq as cp

  1. From the package already setup in test space...
  2. In the ecf script, comment out the two lines above and add
cpreq() {
  /usr/bin/cp "$@"
}
export -f cpreq
declare -f cpreq
  1. Run job, check output files to confirm that /usr/bin/cp was called in place of cpreq and that job completed successfully

Results

The following output files are available on Dogwood.

epoch.v1.0.7
With new cpreq: /lfs/h1/ops/test/output/20251105/epoch_00.o77381507
With cp substituted: /lfs/h1/ops/test/output/20251106/epoch_00.o77547165

ecmwf_mos.v1.2.3
With new cpreq

  • jecmwf_mos_prep: /lfs/h1/ops/test/output/20251105/ecmwf_mos_prep_00.o77375972
  • jecmwf_mos_prep23: /lfs/h1/ops/test/output/20251105/ecmwf_mos_prep23_00.o77376625
  • jecmwf_mos_ext_prep23: /lfs/h1/ops/test/output/20251105/ecmwf_mos_ext_prep23_00.o77377803
  • jecmwf_mos_ext_prep: /lfs/h1/ops/test/output/20251105/ecmwf_mos_ext_prep_00.o77378347
  • jecmwf_mos_stations_ens_fcst: /lfs/h1/ops/test/output/20251106/ecmwf_mos_stations_ens_fcst_00.o77547920

With cp substituted:

  • jecmwf_mos_prep: /lfs/h1/ops/test/output/20251106/ecmwf_mos_prep_00.o77556285
  • jecmwf_mos_prep23: /lfs/h1/ops/test/output/20251106/ecmwf_mos_prep23_00.o77556745
  • jecmwf_mos_ext_prep23: /lfs/h1/ops/test/output/20251106/ecmwf_mos_ext_prep23_00.o77557583
  • jecmwf_mos_ext_prep: /lfs/h1/ops/test/output/20251106/ecmwf_mos_ext_prep_00.o77557185
  • jecmwf_mos_stations_ens_fcst: /lfs/h1/ops/test/output/20251106/ecmwf_mos_stations_ens_fcst_00.o77558257

Summary

The proposed version of cpreq in this PR was tested against two existing production packages, epoch.v1.0.7 and ecmwf_mos.v1.2.3. All tested jobs passed with the new cpreq and with substituting cp in place of cpreq.

@RussellManser-NCO

Copy link
Copy Markdown
Author

Follow-up on discussion from @CarlosMDiaz-NCO regarding the comment from @StevenEarle-NCO above. We could start an aliases "library" for developers to make it easier for them to use some utilities during development. For example, dev_aliases.sh could include the following:

cpreq() {
    filtered_args=("${@//\-z/}")
    cmd="cp ${filtered_args[*]}"
    $cmd
}

export -f cpreq

which could be sourced in a developer's .bashrc.

@RussellManser-NCO

Copy link
Copy Markdown
Author

This version of cpreq has been running in para for 12 days with no observed impacts. I searched output files for potential errors or warnings related to cpreq with the results logged here: /lfs/h1/nco/idsb/noscrub/russell.manser/projects/prod_util/grep.cpreq/grep.cpreq.20251118.log.

I still do not have solid evidence that this would work without issues in operations. Specifically, it isn't clear which packages might expect to copy empty files. I have started searching for which packages use cpreq to pick up data from DCOM, packages that use touch or packages which otherwise redirect output to COM. I found many packages redirect output to COM in many different ways, making an exhaustive search time consuming.

I am open to suggestions that would help discover which packages expect to copy potentially empty files. I think it would be wise to figure this out before moving this change into production.

@RussellManser-NCO

Copy link
Copy Markdown
Author

Given the potential impact this could have on operations and uncertainty around which packages or jobs it could affect, I have an idea to propose.

We could roll out this version of cpreq but instead of throwing an error, it warns of a future change to cpreq if a 0-byte file is copied. This would give NCO time to discover which jobs/packages might need to use cpreq -z and it would give developers a chance to adopt the new option. However, that means we would have to update cpreq again in the future to completely solve the problem at hand.

Thoughts?

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