Skip to content

3576 update fab extraction - #3580

Open
hiker wants to merge 31 commits into
masterfrom
3576_update_fab_extraction
Open

hiker wants to merge 31 commits into
masterfrom
3576_update_fab_extraction

Conversation

@hiker

@hiker hiker commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Update the extraction integration test to:

  • Use Fab directly (instead of the old BAF directory). It does need an unreleased version of Fab, again waiting for UKMO to merge. Once there was the next Fab release, this is easy to update.
  • Use lfric_core and lfric_apps git version instead of SVN. For now, this means that I have to use branches in my forked lfric_core and lfric_appc repository, until the UKMO has integrated all PRs into trunk. Then it should be a simple change to just use the corresponding hash.
  • I compile and test two drivers now (just because I can, no real reason :) ). I had to add some gfortran compilation flags (allow-argument-mismatch).

I needed to include some additional scripts, and have put them in .github/workflow/fab_scripts (I doubt that UKMO wants the extraction scripts in the main repo). I copy these scripts into the checked-out lfric core and apps repositories.

I also believe the file examples/lfric/scripts/extract_script.py is not needed anymore (I've added a copy to the fab_scripts, so consistently all IT related Fab files are in one directory).

I am not sure why lfric_test.yml does git config ... insteadOf... - that line did not work for me at all (aborted with missing key), and it all works without this line.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (e93a73c) to head (e875c4a).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #3580   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          403       403           
  Lines        56790     56790           
=========================================
  Hits         56790     56790           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sergisiso
sergisiso deployed to integration September 7, 2026 11:46 — with GitHub Actions Active
@sergisiso

sergisiso commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

@hiker The ITs of 3.13 succeeded but with 3.14 (my last commit) failed with:

  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/fab/steps/preprocess.py", line 90, in pre_processor
    results = run_mp(config, items=mp_args, func=process_artefact)
  File "/archive/psyclone-tests/action-runner-software/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.14/site-packages/fab/steps/__init__.py", line 51, in run_mp
    results = p.map(func, items)
  File "/apps/packages/python/3.14.7/lib/python3.14/multiprocessing/pool.py", line 367, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/apps/packages/python/3.14.7/lib/python3.14/multiprocessing/pool.py", line 774, in get
    raise self._value
Exception: error preprocessing /archive/psyclone-tests/latest-run/lfric_extraction/lfric_apps/applications/gungho_model/fab_workspace/gungho_model_extract-fast-debug-mpif90-gfortran/source/kernel/inter_function_space/sci_w3_to_w1_average_kernel_mod.F90:
"Profile 'fast-debug' is not defined

Is this something you want to explore? Or should we still keep it pinned to an older python?

@hiker

hiker commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

That is likely a regression, I am checking.

@hiker

hiker commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Looks like a regression. I have simplified some code, which needs class variables, and they are not pickled and setup correctly in subprocesses anymore.

MetOffice/fab#603

@hiker

hiker commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

@sergisiso , while I can fix Fab (see MetOffice/fab#603 for details, I basically keep track of all instances of a class, and copy a class variable into an attribute of each instance ... which then gets distributed properly to all child processes) ... the big problem is that then fparser fails.

I don't get any clear indication of what the problem is, the errors I see are all like this (for any parsed file):

error parsing file '/home/joerg/work/ukmo/core/applications/skeleton/fab-workspace/skeleton-full-debug-mpif90-gfortran/source/algorithm/solver/sci_field_vector_mod.x90':
unhandled error '<class 'KeyError'>' in /home/joerg/work/ukmo/core/applications/skeleton/fab-workspace/skeleton-full-debug-mpif90-gfortran/source/algorithm/solver/sci_field_vector_mod.x90
'End_Program_Stmt'

error parsing file '/home/joerg/work/ukmo/core/applications/skeleton/fab-workspace/skeleton-full-debug-mpif90-gfortran/source/algorithm/sci_fem_constants_mod.x90':
unhandled error '<class 'KeyError'>' in /home/joerg/work/ukmo/core/applications/skeleton/fab-workspace/skeleton-full-debug-mpif90-gfortran/source/algorithm/sci_fem_constants_mod.x90
'End_Program_Stmt'

I am not that deep into the classes of fparser, and if there are any class variable that are modified. @sergisiso , can you confirm if this is the case (which would explain the error: the child processes only get the value of class variables at import time, NOT any modified values that are set later).

The other solution that works would be to switch Fab back to use fork (fork was deprecated as the default due to safety issues such as deadlocks when multi-threading or interacting with external C libraries/accelerators). With this small change, it all works for me.

@hiker
hiker deployed to integration September 8, 2026 02:11 — with GitHub Actions Active
@hiker

hiker commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

I've created a (temporary) branch on Fab that uses fork, and then the extraction test with python 3.14 succeeds. I'll get in touch with the people responsible for Fab to check if that is acceptable. If not, I am happy to fix the Fab issue, it's annoying not hard ... but then we need to do something about fparser :(

@arporter

arporter commented Sep 9, 2026

Copy link
Copy Markdown
Member

I think that modifying classes is fundamental to the way fparser works (stfc/fparser#191).

@hiker

hiker commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

OK, I take that as additional argument that Fab should just switch to the old subprocess behaviour (i.e. fork) - since otherwise Fab will never work with Python 3.14.

My suggestion would then be to switch this back to 3.14, and once we have the next Fab release (which should include a fix for this), we switch Fab to use this specific version, and bump up python to 3.14.

@hiker
hiker deployed to integration September 15, 2026 22:53 — with GitHub Actions Active
@hiker
hiker deployed to integration September 24, 2026 02:19 — with GitHub Actions Active
@hiker

hiker commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

We just had a Fab 2.3.0 release done. I've updated the PR to use this latest release, and brought it up to master. Ready for next review. The IT just passed (though I did only run the extraction one, since this PR does not touch anything else).

@hiker

hiker commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Forgot to update to 3.14, so rerun IT - and just in case ran them all, so this is really ready :)

This branch was successfully deployed

1 active deployment
integration e875c4a6 Deployed Sep 24, 2026 by hiker via build #1832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants