Promote PBS constants to optional parameters by SeanBryan51 · Pull Request #121 · CABLE-LSM/benchcab

SeanBryan51 · 2023-07-28T04:25:37Z

This change promotes PBS related constants to optional parameters in the configuration file so that PBS flags can be set at runtime.

This is useful in running multiple benchcab instances with different job parameters such as memory and the number of CPUs. This will also allow us to easily find an optimal number of CPUs to use to maximise performance.

This change also adds the ability to switch on and off multiprocessing at runtime via an optional parameter in the config file.

Fixes #104

codecov · 2023-07-28T04:28:42Z

Codecov Report

Merging #121 (309adca) into master (b5c2169) will decrease coverage by 0.19%.
Report is 1 commits behind head on master.
The diff coverage is 81.18%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   88.44%   88.26%   -0.19%     
==========================================
  Files          27       27              
  Lines        1385     1474      +89     
==========================================
+ Hits         1225     1301      +76     
- Misses        160      173      +13

Files Changed	Coverage Δ
benchcab/benchcab.py	`33.91% <0.00%> (-2.80%)`	⬇️
benchcab/comparison.py	`61.36% <0.00%> (ø)`
tests/common.py	`88.57% <ø> (ø)`
benchcab/task.py	`83.00% <50.00%> (ø)`
benchcab/bench_config.py	`100.00% <100.00%> (ø)`
benchcab/internal.py	`90.24% <100.00%> (-0.24%)`	⬇️
benchcab/utils/pbs.py	`100.00% <100.00%> (ø)`
tests/test_bench_config.py	`100.00% <100.00%> (ø)`
tests/test_pbs.py	`100.00% <100.00%> (ø)`

SeanBryan51 · 2023-07-31T01:44:03Z

Integration tests

Test multiprocessing set to False runs fluxsite tasks in serial:

#!/bin/bash
bench_example_dir='bench_example_test_multiprocessing_param'
rm -rf $bench_example_dir
git clone git@github.com:CABLE-LSM/bench_example.git $bench_example_dir
cd $bench_example_dir
git reset --hard 6287539e96fc8ef36dc578201fbf9847314147fb
sed -i 's/project:.*/project: tm70/g' config.yaml
sed -i 's/experiment:.*/experiment: AU-Tum/g' config.yaml
sed -i 's/ccc561\/v3.0-YP-changes/sb8430\/test-branch/g' config.yaml
echo "
fluxsite:
  multiprocess: False
" >> config.yaml
echo "
science_configurations:
  - cable:
      cable_user:
        CONSISTENCY_CHECK: False
" >> config.yaml
benchcab run -v

Job standard output:

Running fluxsite tasks...
Running task AU-Tum_2002-2017_OzFlux_Met_R0_S0... CABLE standard output saved in /scratch/tm70/sb8430/benchcab_integration_tests/bench_example_test_multiprocessing_param/runs/fluxsite/tasks/AU-Tum_2002-2017_OzFlux_Met_R0_S0/out.txt
./cable cable.nml
Adding attributes to output file: /scratch/tm70/sb8430/benchcab_integration_tests/bench_example_test_multiprocessing_param/runs/fluxsite/outputs/AU-Tum_2002-2017_OzFlux_Met_R0_S0_out.nc
Running task AU-Tum_2002-2017_OzFlux_Met_R1_S0... CABLE standard output saved in /scratch/tm70/sb8430/benchcab_integration_tests/bench_example_test_multiprocessing_param/runs/fluxsite/tasks/AU-Tum_2002-2017_OzFlux_Met_R1_S0/out.txt
./cable cable.nml
Adding attributes to output file: /scratch/tm70/sb8430/benchcab_integration_tests/bench_example_test_multiprocessing_param/runs/fluxsite/outputs/AU-Tum_2002-2017_OzFlux_Met_R1_S0_out.nc
Successfully ran fluxsite tasks

Running comparison tasks...
Comparing files AU-Tum_2002-2017_OzFlux_Met_R0_S0_out.nc and AU-Tum_2002-2017_OzFlux_Met_R1_S0_out.nc bitwise...
nccmp -df /scratch/tm70/sb8430/benchcab_integration_tests/bench_example_test_multiprocessing_param/runs/fluxsite/outputs/AU-Tum_2002-2017_OzFlux_Met_R0_S0_out.nc /scratch/tm70/sb8430/benchcab_integration_tests/bench_example_test_multiprocessing_param/runs/fluxsite/outputs/AU-Tum_2002-2017_OzFlux_Met_R1_S0_out.nc
Success: files AU-Tum_2002-2017_OzFlux_Met_R0_S0_out.nc AU-Tum_2002-2017_OzFlux_Met_R1_S0_out.nc are identical
Successfully ran comparison tasks

======================================================================================
                  Resource Usage on 2023-08-07 20:45:40:
   Job Id:             92144995.gadi-pbs
   Project:            tm70
   Exit Status:        0
   Service Units:      1.79
   NCPUs Requested:    18                     NCPUs Used: 18
                                           CPU Time Used: 00:02:54
   Memory Requested:   30.0GB                Memory Used: 199.62MB
   Walltime requested: 06:00:00            Walltime Used: 00:02:59
   JobFS requested:    100.0MB                JobFS used: 0B
======================================================================================

Test ncpus, mem and walltime set the PBS flags correctly in the job script:

#!/bin/bash
bench_example_dir='bench_example_test_pbs_params'
rm -rf $bench_example_dir
git clone git@github.com:CABLE-LSM/bench_example.git $bench_example_dir
cd $bench_example_dir
git reset --hard 6287539e96fc8ef36dc578201fbf9847314147fb
sed -i 's/project:.*/project: tm70/g' config.yaml
sed -i 's/ccc561\/v3.0-YP-changes/sb8430\/test-branch/g' config.yaml
echo "
fluxsite:
  pbs:
    ncpus: 20
    mem: 20GB
    walltime: 01:00:00
" >> config.yaml
benchcab run -v

Job summary:

======================================================================================
                  Resource Usage on 2023-07-31 12:58:25:
   Job Id:             91614874.gadi-pbs
   Project:            tm70
   Exit Status:        0
   Service Units:      4.12
   NCPUs Requested:    20                     NCPUs Used: 20
                                           CPU Time Used: 01:28:51
   Memory Requested:   20.0GB                Memory Used: 3.44GB
   Walltime requested: 01:00:00            Walltime Used: 00:06:11
   JobFS requested:    100.0MB                JobFS used: 0B
======================================================================================

ccarouge

Great work. Just thinking we might need options to differentiate between fluxsite and spatial.

ccarouge · 2023-08-01T14:50:34Z

+    # the "pbs" key is optional
+    if "pbs" in config:


I think we will need 2 PBS keys: one for fluxsite and one for spatial as the resources are likely going to be quite different.

So should we rename "pbs" to "pbs_fluxsite" ?

Yes I agree. I'm thinking we go with this structure:

fluxsite: pbs: ncpus: 16 ... multiprocessing: True spatial: pbs: ncpus: 16 ...

mainly because multiprocessing is probably specific to only fluxsite tests. My guess is we won't be running multiple cable MPI executables in parallel on a single PBS job like we do with the fluxsite case (e.g. if we use payu run to execute each spatial task, we would only be able to run one task per PBS job). Although it might be possible to run multiple MPI instances on a single PBS job (see here for example).

Yes, that looks good.

ccarouge · 2023-08-01T14:54:20Z

+    # the "multiprocessing" key is optional
+    if "multiprocessing" in config and not isinstance(config["multiprocessing"], bool):


Again, do we need 2: one for fluxsite and one for spatial? Not sure here, so we could leave as is for now.

As per reply above

multiprocessing is probably specific to only fluxsite tests. My guess is we won't be running multiple cable MPI executables in parallel on a single PBS job like we do with the fluxsite case (e.g. if we use payu run to execute each spatial task, we would only be able to run one task per PBS job). Although it might be possible to run multiple MPI instances on a single PBS job (see here for example).

ccarouge · 2023-08-01T14:57:57Z

-NCPUS = 18
-MEM = "30GB"
-WALL_TIME = "6:00:00"
+DEFAULT_PBS: Any = {"ncpus": 18, "mem": "30GB", "walltime": "6:00:00", "storage": []}


Need 2, one for fluxsite and one for spatial.

This change promotes PBS related constants to optional parameters in the configuration file so that PBS flags can be set at runtime. This is useful in running multiple benchcab instances with different job parameters such as memory and the number of CPUs. This will also allow us to easily find an optimal number of CPUs to use to maximise performance. This change also adds the ability to switch on and off multiprocessing at runtime via an optional parameter in the config file. Fixes #104

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

Co-authored-by: Claire Carouge <claire.carouge@anu.edu.au>

In config.yaml options, 'multiprocessing' should be 'multiprocess'.

ccarouge

Just a couple of suggestions for the documentation to specify fluxsite and pbs are optional.

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

SeanBryan51 linked an issue Jul 28, 2023 that may be closed by this pull request

Promote PBS related constants to parameters in the config file #104

Closed

SeanBryan51 force-pushed the 104-promote-pbs-related-constants-to-parameters-in-the-config-file branch from 08c04bb to 4d07515 Compare July 28, 2023 05:34

SeanBryan51 requested a review from ccarouge July 31, 2023 03:40

ccarouge requested changes Aug 1, 2023

View reviewed changes

SeanBryan51 and others added 6 commits August 7, 2023 15:51

Update docs/user_guide/config_options.md

e47fb64

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

Update docs/user_guide/config_options.md

f0b82bb

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

Update docs/user_guide/config_options.md

f89374c

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

Update docs/user_guide/config_options.md

197b3c3

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

Update docs/user_guide/config_options.md

da2233a

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

SeanBryan51 force-pushed the 104-promote-pbs-related-constants-to-parameters-in-the-config-file branch 2 times, most recently from 06a94be to d5ba907 Compare August 7, 2023 06:34

Add requested changes from code review

03a724c

Co-authored-by: Claire Carouge <claire.carouge@anu.edu.au>

SeanBryan51 force-pushed the 104-promote-pbs-related-constants-to-parameters-in-the-config-file branch from d5ba907 to 03a724c Compare August 7, 2023 10:05

SeanBryan51 requested a review from ccarouge August 7, 2023 10:51

Fix typo in documentation

68bea1b

In config.yaml options, 'multiprocessing' should be 'multiprocess'.

ccarouge approved these changes Aug 10, 2023

View reviewed changes

Comment thread docs/user_guide/config_options.md Outdated

Comment thread docs/user_guide/config_options.md Outdated

SeanBryan51 and others added 2 commits August 10, 2023 17:28

Update docs/user_guide/config_options.md

8c4c799

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

Update docs/user_guide/config_options.md

309adca

Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>

SeanBryan51 merged commit a35753f into master Aug 10, 2023

SeanBryan51 deleted the 104-promote-pbs-related-constants-to-parameters-in-the-config-file branch August 10, 2023 23:34

		# the "multiprocessing" key is optional
		if "multiprocessing" in config and not isinstance(config["multiprocessing"], bool):

Conversation

SeanBryan51 commented Jul 28, 2023

Uh oh!

codecov Bot commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SeanBryan51 commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Integration tests

Uh oh!

ccarouge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ccarouge Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

SeanBryan51 Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

ccarouge Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ccarouge Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

SeanBryan51 Aug 3, 2023 • 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!

ccarouge Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

ccarouge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 28, 2023 •

edited

Loading

SeanBryan51 commented Jul 31, 2023 •

edited

Loading

SeanBryan51 Aug 3, 2023 •

edited

Loading