Deprecate `serial_cable` and `parallel_cable` scripts #193

SeanBryan51 · 2023-12-11T23:04:12Z

This change simplifies the CABLE offline build system by deprecating the serial_cable and parallel_cable build scripts and moving their functionality into the Makefile.

Replace ls *.o in serial_cable and parallel_cable scripts with $(OBJS) in the Makefile where OBJS is a list of object files common to both serial and MPI. This is done so that:

We leverage make syntax to simplify the rules for serial and mpi targets.
We can replace the all target (which contains a list of object files as dependencies) with $(OBJS).
We clean up the existing definition of LSRC in the Makefile and use this to generate a list of the required object files.
See #193 for a sorted diff of the new LSRC variable against the previous LSRC and against the list of object file dependencies in the all target.

Tools for testing binary equivalence of executables (see ldd, nm and objdump tools), show that the serial and MPI executables are equivalent to their respective executable in the main branch.

Fixes #189

📚 Documentation preview 📚: https://cable--193.org.readthedocs.build/en/193/

SeanBryan51

@harshula has tools for checking binary equivalence between executable files, these should be used so we can verify we haven't introduced any unwanted changes from refactoring.

src/offline/serial_cable

src/offline/parallel_cable

src/offline/build3.sh

src/offline/Makefile

JhanSrbinovsky

Docs on building might need corresponding revision

ccarouge

This looks good to me. A couple of minor suggestions on the comments in the Makefile to make them more descriptive.

src/offline/Makefile

ccarouge

Looks good to me.

Just curious: why were added dependencies needed at this point of the work? To fix the order of compilation?

SeanBryan51 · 2023-12-19T01:11:27Z

Changes to `LSRC` variable

A sorted diff of the old LSRC variable vs new is as follows:

--- /home/189/sb8430/LSRC.txt	2023-12-18 13:07:13.869287000 +1100
+++ /home/189/sb8430/LSRC_new.txt	2023-12-19 10:08:10.295839000 +1100
@@ -1,6 +1,4 @@
 bgcdriver
-bgcdriver
-biogeochem_casa
 biogeochem_casa
 cable_abort
 cable_air
@@ -45,7 +43,6 @@
 casa_cnp
 casa_dimension
 casa_feedback
-casa_feedback
 casa_inout
 casa_ncdf
 casa_offline_inout
@@ -55,17 +52,15 @@
 casa_readbiome
 casa_rplant
 casa_sumcflux
-casa_sumcflux
 casa_variable
+cbl_albedo
 cbl_conductivity
 cbl_dryLeaf
 cbl_friction_vel
 cbl_fwsoil
 cbl_GW
-cbl_GW
 cbl_hyd_redistrib
 cbl_init_radiation
-cbl_LAI_canopy_height
 cbl_latent_heat
 cbl_model_driver_offline
 cbl_Oldconductivity
@@ -106,7 +101,6 @@
 POP
 pop_constants
 pop_def
-POPdriver
 pop_io
 POPLUC
 pop_types

This change adds one missing file (cbl_albedo.F90) and removes two files (POPdriver.F90 which no longer exists and cbl_LAI_canopy_height.F90 which is not required for the offline configuration).

A sorted diff of the list of object file dependencies in the all target vs the new LSRC variable is as follows:

--- /home/189/sb8430/common.txt	2023-12-18 13:03:52.769319000 +1100
+++ /home/189/sb8430/LSRC_new.txt	2023-12-19 10:08:10.295839000 +1100
@@ -16,6 +16,7 @@
 cable_iovars
 cable_LUC_EXPT
 cable_maths_constants_mod
+cable_metutils
 cable_namelist_input
 cable_other_constants_mod
 cable_output
@@ -54,11 +55,18 @@
 casa_variable
 cbl_albedo
 cbl_conductivity
+cbl_dryLeaf
+cbl_friction_vel
+cbl_fwsoil
 cbl_GW
 cbl_hyd_redistrib
 cbl_init_radiation
+cbl_latent_heat
 cbl_model_driver_offline
 cbl_Oldconductivity
+cbl_photosynthesis
+cbl_pot_evap_snow
+cbl_qsat
 cbl_radiation
 cbl_remove_trans
 cbl_rhoch
@@ -75,13 +83,16 @@
 cbl_soilfreeze
 cbl_soilsnow_data
 cbl_soilsnow_init_special
-cbl_soilsnow_init_special
 cbl_soilsnow_main
 cbl_spitter
 cbl_stempv
+cbl_SurfaceWetness
 cbl_surfbv
 cbl_thermal
 cbl_trimb
+cbl_wetleaf
+cbl_within_canopy
+cbl_zetar
 grid_constants_cbl
 landuse3
 landuse_constant

The OBJS variable (derived from LSRC) includes files that were missing from the all target. This is because OBJS must contain the source files that are required at the linking step, otherwise the compiler will complain. It is likely that files which were missing were being compiled somewhere along the dependency chain. The OBJS variable also lists the source files in a different order to how it was in the all target. This changes the order of how source files are compiled (hence the changes to *.o dependency rules). Note, this does not ensure that target dependencies for all *.o files are correct. Only the minimal required changes to make the compilation work have been made.

SeanBryan51 · 2023-12-19T01:19:25Z

Looks good to me.

Just curious: why were added dependencies needed at this point of the work? To fix the order of compilation?

The changes to dependencies was a result of using the $(OBJS) instead of the all target. I have updated the PR description and provided an explanation for the diff between OBJS and the object file dependencies in all.

This change simplifies the CABLE offline build system by deprecating the `serial_cable` and `parallel_cable` build scripts and moving their functionality into the Makefile. Replace `ls *.o` in `serial_cable` and `parallel_cable` scripts with `$(OBJS)` in the Makefile where `OBJS` is a list of object files common to both serial and MPI. This is done so that: 1. We leverage `make` syntax to simplify the rules for `serial` and `mpi` targets. 2. We can replace the `all` target (which contains a list of object files as dependencies) with `$(OBJS)`. 3. We clean up the existing definition of `LSRC` in the Makefile and use this to generate a list of the required object files. See [#193][PR] for a sorted diff of the new `LSRC` variable against the previous `LSRC` and against the list of object file dependencies in the `all` target. Tools for testing binary equivalence of executables (see `ldd`, `nm` and `objdump` tools), show that the serial and MPI executables are equivalent to their respective executable in the main branch. Fixes #189 [PR]: #193

harshula

Well done! Good work simplifying the build system and making it more maintainable.

The `serial_cable` and `parallel_cable` scripts were deprecated in CABLE-LSM/CABLE#193. This change does the same in the benchcab source code. For code branches that require the serial_cable and parallel_cable scripts can use the `build_script` key as a work around. Fixes #223

access-hive-bot · 2024-03-07T06:05:09Z

This pull request has been mentioned on ACCESS Hive Community Forum. There might be relevant details there:

https://forum.access-hive.org.au/t/benchcab-python-based-software-for-the-evaluation-of-cable/873/7

SeanBryan51 requested review from JhanSrbinovsky and ccarouge December 11, 2023 23:04

SeanBryan51 linked an issue Dec 11, 2023 that may be closed by this pull request

Move serial_build and parallel_build functionality into Makefile #189

Closed

SeanBryan51 requested a review from harshula December 11, 2023 23:04

SeanBryan51 commented Dec 12, 2023

View reviewed changes

src/offline/serial_cable Outdated Show resolved Hide resolved

src/offline/parallel_cable Outdated Show resolved Hide resolved

src/offline/build3.sh Show resolved Hide resolved

src/offline/Makefile Outdated Show resolved Hide resolved

src/offline/Makefile Outdated Show resolved Hide resolved

JhanSrbinovsky approved these changes Dec 12, 2023

View reviewed changes

ccarouge approved these changes Dec 15, 2023

View reviewed changes

src/offline/Makefile Outdated Show resolved Hide resolved

src/offline/Makefile Outdated Show resolved Hide resolved

SeanBryan51 force-pushed the 189-deprecate-serial_build-and-parallel_build branch 4 times, most recently from 4db3a87 to 6b1a2f7 Compare December 18, 2023 13:09

ccarouge approved these changes Dec 18, 2023

View reviewed changes

SeanBryan51 changed the title ~~Deprecate serial_build and parallel_build scripts~~ Deprecate serial_cable and parallel_cable scripts Dec 19, 2023

SeanBryan51 force-pushed the 189-deprecate-serial_build-and-parallel_build branch from 2ae8556 to 4926bb1 Compare December 19, 2023 02:29

harshula approved these changes Dec 19, 2023

View reviewed changes

SeanBryan51 merged commit 8ba8c9e into main Dec 19, 2023
3 checks passed

SeanBryan51 deleted the 189-deprecate-serial_build-and-parallel_build branch December 19, 2023 04:15

SeanBryan51 mentioned this pull request Dec 20, 2023

Deprecate serial_cable and parallel_cable scripts CABLE-LSM/benchcab#223

Closed

SeanBryan51 mentioned this pull request Dec 20, 2023

Deprecate serial_cable and parallel_cable scripts CABLE-LSM/benchcab#225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `serial_cable` and `parallel_cable` scripts #193

Deprecate `serial_cable` and `parallel_cable` scripts #193

SeanBryan51 commented Dec 11, 2023 •

edited

Loading

SeanBryan51 left a comment

JhanSrbinovsky left a comment

ccarouge left a comment

ccarouge left a comment

SeanBryan51 commented Dec 19, 2023

SeanBryan51 commented Dec 19, 2023 •

edited

Loading

harshula left a comment

access-hive-bot commented Mar 7, 2024

Deprecate serial_cable and parallel_cable scripts #193

Deprecate serial_cable and parallel_cable scripts #193

Conversation

SeanBryan51 commented Dec 11, 2023 • edited Loading

SeanBryan51 left a comment

Choose a reason for hiding this comment

JhanSrbinovsky left a comment

Choose a reason for hiding this comment

ccarouge left a comment

Choose a reason for hiding this comment

ccarouge left a comment

Choose a reason for hiding this comment

SeanBryan51 commented Dec 19, 2023

Changes to LSRC variable

SeanBryan51 commented Dec 19, 2023 • edited Loading

harshula left a comment

Choose a reason for hiding this comment

access-hive-bot commented Mar 7, 2024

Deprecate `serial_cable` and `parallel_cable` scripts #193

Deprecate `serial_cable` and `parallel_cable` scripts #193

SeanBryan51 commented Dec 11, 2023 •

edited

Loading

Changes to `LSRC` variable

SeanBryan51 commented Dec 19, 2023 •

edited

Loading