Add skeleton fab script by hiker · Pull Request #246 · MetOffice/lfric_core

Joerg Henrichs (hiker) · 2026-01-27T06:04:11Z

PR Summary

Sci/Tech Reviewer: Matthew Hambley (@MatthewHambley)
Code Reviewer: Sam Clarke-Green (@t00sa)

Adds a first Fab build script for Skeleton. To keep this change minimal, it's command line only (i.e. no cylc integration, which can come later).

closes Add Fab build script for Skeleton apps #240

Code Quality Checklist

I have performed a self-review of my own code
My code follows the project's style guidelines
Comments have been included that aid understanding and enhance the readability of the code
My changes generate no new warnings
All automated checks in the CI pipeline have completed successfully

Testing

I have tested this change locally, using the LFRic Core rose-stem suite
If required (e.g. API changes) I have also run the LFRic Apps test suite using this branch
If any tests fail (rose-stem or CI) the reason is understood and acceptable (e.g. kgo changes)
I have added tests to cover new functionality as appropriate (e.g. system tests, unit tests, etc.)
Any new tests have been assigned an appropriate amount of compute resource and have been allocated to an appropriate testing group (i.e. the developer tests are for jobs which use a small amount of compute resource and complete in a matter of minutes)

trac.log

Security Considerations

I have reviewed my changes for potential security issues
Sensitive data is properly handled (if applicable)
Authentication and authorisation are properly implemented (if applicable)

Performance Impact

Performance of the code has been considered and, if applicable, suitable performance measurements have been conducted

AI Assistance and Attribution

Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy (including attribution labels)

Documentation

Where appropriate I have updated documentation related to this change and confirmed that it builds correctly

PSyclone Approval

If you have edited any PSyclone-related code (e.g. PSyKAl-lite, Kernel interface, optimisation scripts, LFRic data structure code) then please contact the TCD Team

Sci/Tech Review

I understand this area of code and the changes being added
The proposed changes correspond to the pull request description
Documentation is sufficient (do documentation papers need updating)
Sufficient testing has been completed

(Please alert the code reviewer via a tag when you have approved the SR)

Code Review

All dependencies have been resolved
Related Issues have been properly linked and addressed
CLA compliance has been confirmed
Code quality standards have been met
Tests are adequate and have passed
Documentation is complete and accurate
Security considerations have been addressed
Performance impact is acceptable

…ler.

…nfiguration to compile skeleton.

…ent.

Joerg Henrichs (hiker) · 2026-01-27T07:34:16Z

This PR only adds a command-line build system using Fab for the Skeleton apps. It is not at all integrated into cylc or any other test suite. Tests have been added to the infrastructure/build/fab scripts which cover the newly added infrastructure files there.

Some unit tests are based on some AI input to setup the frame work, but have been manually tweaked to ensure code coverage.

Documentation is in form of a README in the above directory, please let me know if you want me to add more elsewhere.

Matthew Hambley (MatthewHambley)

I note that a lot of requests made in the original review on SRS have not been addressed so for convenience I have repeated them here.

…ot).

Joerg Henrichs (hiker) · 2026-02-09T06:41:43Z

I note that a lot of requests made in the original review on SRS have not been addressed so for convenience I have repeated them here.

I note that all my comments and questions to your original reviews (as originally agreed a few months ago on MetOffice/lfric-baf#83) have not been addressed so for convenience I will repeat them here.

It will take me a while to get through all your comments, but I will start adding comments now, so ideally we can discuss some of the issue and reach an agreement while I still work on other comments.

Matthew Hambley (MatthewHambley)

This change represents an initial implementation on which we can build.

Sam Clarke-Green (t00sa)

Thanks, looks good. See comments inline for minor changes.

Sam Clarke-Green (t00sa) · 2026-03-23T14:25:29Z

+            return
+
+    if gfortran.get_version() < (4, 9):
+        raise RuntimeError(f"GFortran is too old to build dynamo. "


Minor name change:

Suggested change

raise RuntimeError(f"GFortran is too old to build dynamo. "

raise RuntimeError(f"GFortran is too old to build LFRic. "

Sam Clarke-Green (t00sa) · 2026-03-23T16:21:38Z

+## Setting up Site- and Platform-specific Settings
+Site- and platform-specific settings are contained in 
+```$LFRIC_CORE/infrastructure/build/site-specific/${SITE}-${PLATFORM}```
+The default settings are in ```.../site-specific/default``` (and at this


Suggested change

The default settings are in ```.../site-specific/default``` (and at this

The default settings are in ```../site-specific/default``` (and at this

Hmm - I have to admit I don't like this: .. indicates the parent directory, so I fear that this might be understood as site_specific being lfric_core/site_specific, while it is in lfric_core/lfric_build/site_specific.

What about removing the .../ entirely ("... are in site_specific/default"). Or "are in $LFRIC_CORE/lfric_build/site-specific" instead?

For now I've removed the ``.../```, hope that's a good compromise.

Sam Clarke-Green (t00sa) · 2026-03-24T08:21:17Z

+and use the following command:
+
+```
+./fab_skeleton.py --nprocs 4 --site nci --platform gadi  --suite intel-classic


This should probably be a more generic example. Site flags should be documented separately if they are not required.

Suggested change

./fab_skeleton.py --nprocs 4 --site nci --platform gadi --suite intel-classic

./fab_skeleton.py --nprocs 4

Hmm - I don't like removing this entirely, since it would often end up in using gfortran, while sites might expect intel compiler to be used (if no compiler is specified, fab will pick the first available one in its internal list, which is alphabetically sorted). So, I took --site and --platform out, but left the --suite in, together with a list of the default valid suite names.

Then I added a paragraph to discuss the site-specific setup (which will likely be required, since libraries will likely need additional flags). I hope that's ok.

Sam Clarke-Green (t00sa) · 2026-03-24T08:57:02Z

This is not required. Please delete.

Sam Clarke-Green (t00sa) · 2026-03-24T10:21:09Z

+        self.setup_gnu(build_config)
+        self.setup_nvidia(build_config)
+        self.setup_cray(build_config)
+


All the lfric applications currently require netCDF. Would be a good point to check whether any netcdf flags are set and trigger an immediate error rather than waiting for the linker to fail?

Difficult question. Yes, everything needs NetCDF. But the default setup will create a netcdf entry (based on nf-config, though some sites might overwrite this due to issues with nf-config on Crays). So, each linker will have a netcdf entry.

We could test if an entry for netcdf is empty (i.e. no flags), which happens when nf-config is not available. BUT, not specifying flags for NetCDF is de-facto valid on Cray systems (I assume that is still the case, that loading the netcdf module will automatically add the required include and linking flags).

Additionally, typically an application will not explicitly specify a linker (in which case the linking step will pick a linker based on compiler-suite, mpi flags and language of the main program, i.e. Fortran or C). So, before doing the analysis step we will likely not even know which linker to use (since only the analysis step will determine program units).

Now, this is a lot of theoretical thinking of setups, which will likely not happen (I can't see a good reason to link with any LFRic apps with a C compiler wrapper). I checked, on ncas_ex we hard-coded explicit libraries for NetCDF (since nf-config there did not work). So, I've added the test that you recommended, which will abort before starting the actual work.

…script

Joerg Henrichs (hiker) · 2026-04-09T06:42:37Z

I've addressed the issues raised here (though in some cases differently, and am happy to discuss). Brought up to current main; ready for next review.

Sam Clarke-Green (t00sa) · 2026-04-29T10:55:31Z

Can you go through and add copyright headers to all the files that need them?

Sam Clarke-Green (t00sa)

I've added some suggestions based on changes to the UM versions of similar files.

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

…script

Joerg Henrichs (hiker) · 2026-05-04T02:05:42Z

Can you go through and add copyright headers to all the files that need them?

Thanks, done. I did notice that there seems to be two different templates, one with year, one without. I used the one with year, not sure what you prefer.

Joerg Henrichs (hiker) · 2026-05-04T02:06:00Z

I've added some suggestions based on changes to the UM versions of similar files.

All applied, thanks a lot!!

Sam Clarke-Green (t00sa)

Looks good. Approved

…b_script

Sam Clarke-Green (t00sa)

Re-approving.

Joerg Henrichs (hiker) added 11 commits January 23, 2026 17:40

MetOffice#240 Added new base class for Fab scripts.

4dbac71

MetOffice#240 Removed support for transmute step to keep this PR smal…

2254b31

…ler.

MetOffice#240 Add all skeleton fab script, helper classes and site-co…

c1e87d1

…nfiguration to compile skeleton.

MetOffice#240 Updated tests to pass all linters, fixed licence statem…

a350ef3

…ent.

MetOffice#240 Updated skeleton build script.

962ce0a

MetOffice#240 Fixed incorrect typing in test.

54491da

MetOffice#240 Added README file.

22c2ef9

MetOffice#240 Removed fab and vernier support from NCI site.

25c26fb

MetOffice#240 Fixed typos.

72192df

MetOffice#240 Allow import of psyclone_tools without setting PYTHONPATH.

f7a839c

MetOffice#240 Fixed flake8 errors.

170cab3

Joerg Henrichs (hiker) requested review from Matthew Hambley (MatthewHambley), Mike Hobson (mike-hobson) and Steve Mullerworth (stevemullerworth) as code owners January 27, 2026 06:04

Joerg Henrichs (hiker) marked this pull request as draft January 27, 2026 06:04

github-actions Bot added the cla-required The CLA has not yet been signed by the author of this PR - added by GA label Jan 27, 2026

MetOffice#240 Added myself to contributors list.

d7aa9f1

github-actions Bot added cla-signed The CLA has been signed as part of this PR - added by GA and removed cla-required The CLA has not yet been signed by the author of this PR - added by GA labels Jan 27, 2026

Merge branch 'main' into 240_add_skeleton_fab_script

2315007

Joerg Henrichs (hiker) marked this pull request as ready for review January 27, 2026 07:37

Yaswant Pradhan (yaswant) requested review from Sam Clarke-Green (t00sa) and removed request for Mike Hobson (mike-hobson) and Steve Mullerworth (stevemullerworth) February 5, 2026 09:26

Matthew Hambley (MatthewHambley) requested changes Feb 5, 2026

View reviewed changes

Joerg Henrichs (hiker) added 2 commits February 8, 2026 11:25

Fixed rose picker etc issues raised in review.

9ee0b64

Renamed test directory to tests (to avoid clash with .gitignore in ro…

6dc4bd8

…ot).

MetOffice#240 Renamed rose_picker_tool.py to rose_picker.py etc.

00b7ec9

Matthew Hambley (MatthewHambley) approved these changes Mar 19, 2026

View reviewed changes

Sam Clarke-Green (t00sa) changed the title ~~240 add skeleton fab script~~ Add skeleton fab script Mar 19, 2026

Merge branch 'main' into 240_add_skeleton_fab_script

5ae6376

This was referenced Mar 31, 2026

Add Fab build script for gravity wave application MetOffice/lfric_apps#407

Open

Fab pFunit support #323

Merged

add fab for gravity wave MetOffice/lfric_apps#411

Open

Sam Clarke-Green (t00sa) reviewed Apr 7, 2026

View reviewed changes

Sam Clarke-Green (t00sa) mentioned this pull request Apr 7, 2026

Unify flag handling MetOffice/fab#504

Merged

Joerg Henrichs (hiker) added 3 commits April 9, 2026 16:32

MetOffice#240 Addressed issues raised in review.

280268b

MetOffice#240 Check that NetCDF is available before starting to compile.

ecdff1d

Merge remote-tracking branch 'origin/main' into 240_add_skeleton_fab_…

cf5b686

…script

Joerg Henrichs (hiker) requested a review from Sam Clarke-Green (t00sa) April 9, 2026 06:42

Sam Clarke-Green (t00sa) requested changes Apr 29, 2026

View reviewed changes

Comment thread lfric_build/site_specific/default/setup_script_gnu.py Outdated

Comment thread lfric_build/site_specific/default/setup_script_gnu.py

Comment thread lfric_build/site_specific/default/config.py Outdated

Comment thread lfric_build/site_specific/default/config.py Outdated

Joerg Henrichs (hiker) and others added 6 commits May 4, 2026 11:03

Update lfric_build/site_specific/default/setup_script_gnu.py

ed7203e

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

Update lfric_build/site_specific/default/setup_script_gnu.py

6ea9af2

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

Update lfric_build/site_specific/default/config.py

513da0b

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

Update lfric_build/site_specific/default/config.py

7bf6c2a

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

MetOffice#240 Added copyright statement.

101ea01

Merge remote-tracking branch 'origin/main' into 240_add_skeleton_fab_…

bd21fdb

…script

Sam Clarke-Green (t00sa) approved these changes May 5, 2026

View reviewed changes

Sam Clarke-Green (t00sa) added 2 commits May 5, 2026 09:57

Merge remote-tracking branch 'upstream/main' into 240_add_skeleton_fa…

e8c84ce

…b_script

Add fab-workspace to gitignore

97490d7

Sam Clarke-Green (t00sa) approved these changes May 5, 2026

View reviewed changes

Sam Clarke-Green (t00sa) merged commit 18ccb42 into MetOffice:main May 5, 2026
7 checks passed

James Bruten (james-bruten-mo) added this to the Summer 2026 milestone May 29, 2026

Joerg Henrichs (hiker) deleted the 240_add_skeleton_fab_script branch June 10, 2026 04:38

	raise RuntimeError(f"GFortran is too old to build dynamo. "
	raise RuntimeError(f"GFortran is too old to build LFRic. "

	The default settings are in ```.../site-specific/default``` (and at this
	The default settings are in ```../site-specific/default``` (and at this

	./fab_skeleton.py --nprocs 4 --site nci --platform gadi --suite intel-classic
	./fab_skeleton.py --nprocs 4

Conversation

Joerg Henrichs (hiker) commented Jan 27, 2026 • edited by MatthewHambley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Code Quality Checklist

Testing

trac.log

Security Considerations

Performance Impact

AI Assistance and Attribution

Documentation

PSyclone Approval

Sci/Tech Review

Code Review

Uh oh!

Joerg Henrichs (hiker) commented Jan 27, 2026

Uh oh!

Matthew Hambley (MatthewHambley) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Joerg Henrichs (hiker) commented Feb 9, 2026

Uh oh!

Matthew Hambley (MatthewHambley) left a comment

Choose a reason for hiding this comment

Uh oh!

Sam Clarke-Green (t00sa) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joerg Henrichs (hiker) commented Apr 9, 2026

Uh oh!

Sam Clarke-Green (t00sa) commented Apr 29, 2026

Uh oh!

Sam Clarke-Green (t00sa) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Joerg Henrichs (hiker) commented May 4, 2026

Uh oh!

Joerg Henrichs (hiker) commented May 4, 2026

Uh oh!

Sam Clarke-Green (t00sa) left a comment

Choose a reason for hiding this comment

Uh oh!

Sam Clarke-Green (t00sa) left a comment

Choose a reason for hiding this comment

Joerg Henrichs (hiker) commented Jan 27, 2026 •

edited by MatthewHambley

Loading