Skip to content

Localize OpenMP thread use and count to the GCs theaded with mini-GS approach#4876

Closed
aoloso wants to merge 5 commits into
release/v2from
bugfix/aoloso/gridComp_level_numthrds
Closed

Localize OpenMP thread use and count to the GCs theaded with mini-GS approach#4876
aoloso wants to merge 5 commits into
release/v2from
bugfix/aoloso/gridComp_level_numthrds

Conversation

@aoloso
Copy link
Copy Markdown
Contributor

@aoloso aoloso commented May 12, 2026

Types of change(s)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)
  • Refactor (no functional changes, no api changes)

Checklist

  • Tested this change with a run of GEOSgcm
  • Ran the Unit Tests (make tests)

Description

For GWD and GOCART2G threaded with the mini-gridcomp approach, the ability to selectively turn OpenMP off for those components where OpenMP is required for other components is not working as inteneded. The runtime option use_threads: .FALSE. to achieve that is being ignored. A branch called bugfix/aoloso/gridComp_level_numthrds has been created fro MAPL, GEOSgcm_GridComp, and GOCART to resolve this bug.

The affected files are MAPL.F90, MAPL_Generic.F90, MaplGenericComponent.F90, OpenMP_Support.F90, GOCART2G_GridCompMod.F90, CA2G_GridCompMod.F90, DU2G_GridCompMod.F90, NI2G_GridCompMod.F90, SU2G_GridCompMod.F90 GEOS_GwdGridComp.F90.

Related Issue

1411

@aoloso aoloso requested a review from a team as a code owner May 12, 2026 20:55
@aoloso aoloso added the 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. label May 12, 2026
@mathomp4 mathomp4 changed the base branch from main to develop May 13, 2026 14:06
@mathomp4
Copy link
Copy Markdown
Member

@aoloso I changed the base to be to develop as it was changing too many files.

@aoloso
Copy link
Copy Markdown
Contributor Author

aoloso commented May 13, 2026

@copilot : What do you think about this PR?

…all set_num_threads

Agent-Logs-Url: https://github.com/GEOS-ESM/MAPL/sessions/73519c55-cf07-416e-be59-296d5124a54e

Co-authored-by: aoloso <8182398+aoloso@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI commented May 13, 2026

@copilot : What do you think about this PR?

Here's my assessment of this PR:

Overall approach: ✅ Correct and clean

The core fix is sound: instead of calling the global omp_get_max_threads() to determine the thread count for a component's !$omp parallel region, each MaplGenericComponent now stores its own num_threads (defaulting to 1) as an instance field. The !$omp& num_threads(num_threads) clause added to the parallel directive ensures the OMP region respects the per-component value. This is exactly the right way to isolate thread counts per GC when using the mini-GS approach.

CI Failure: not related to this PR
The Build Tests failure (get_target_property() called with non-existent target "FMS::fms_r4" in GEOSmoist_GridComp/CMakeLists.txt:70) is a GEOSgcm CMake dependency issue unrelated to the MAPL code changes here. The develop branch build tests pass.

Issues I noticed and fixed:

  • Tests/GetHorzIJIndex/GridComp.F90 declared num_threads as a local variable in setservices but never assigned or used it — fixed in commit dc0fc02 by reading num_threads: from config and calling MAPL%set_num_threads(num_threads), which also completes the usage demo for the new API.

A couple of things to be aware of:

  1. Behavior change for existing threaded components: Any component that currently sets use_threads=.TRUE. but does not explicitly call set_num_threads() will now run with 1 OMP thread (the new default) instead of omp_get_max_threads(). The intent seems to be that GOCART2G/GWD components in their respective repos will call set_num_threads, but existing components relying on the old implicit behavior will silently become single-threaded.
  2. MAPL_get_num_threads removed from public API: This is a breaking change for any downstream code using that symbol. Worth noting in the changelog/PR description.

The changes in the MAPL repo are minimal and well-scoped. The bulk of the fix (calling set_num_threads in GOCART2G, GWD, etc.) lives in the companion PRs for those repos.

Copilot finished work on behalf of aoloso May 13, 2026 14:27
@mathomp4
Copy link
Copy Markdown
Member

The CI error is interesting here. The issue is that the other branches for this by @aoloso are based on GEOSgcm v12. But in MAPL, our CI is currently still based on GEOSgcm v11 (since that is what GEOSgcm main is).

So this is failing since it's checking out the "wrong" GEOSgcm_GridComp, say, from GEOS-ESM/GEOSgcm_GridComp#1412

tclune
tclune previously approved these changes May 13, 2026
@mathomp4
Copy link
Copy Markdown
Member

Okay. I was wrong. This does change an interface in a way:

/discover/nobackup/mathomp4/SystemTests/builds/AGCM_V12/CURRENT/GEOSgcm/src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSgwd_GridComp/GEOS_GwdGridComp.F90(134): error #6404: This name does not have a type, and must have an explicit type.   [MAPL_GET_NUM_THREADS]
      num_threads = MAPL_get_num_threads()
--------------------^

So we might need to coordinate with @sdrabenh

@tclune
Copy link
Copy Markdown
Collaborator

tclune commented May 13, 2026

@mathomp4 You do it as 3 PRs. PR1 in MAPL adds the interface. PR2 changes the client code to use PR1. PR3 eliminates old interface.

Changes are small so you can probably use superpowers to merge before waiting on all CI.

@mathomp4 mathomp4 changed the base branch from develop to release/v2 May 15, 2026 16:26
@aoloso aoloso closed this May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0 Diff The changes in this pull request have verified to be zero-diff with the target branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants