Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove copy of state into mpi window #735

Merged
merged 13 commits into from
Nov 7, 2024
Merged

remove copy of state into mpi window #735

merged 13 commits into from
Nov 7, 2024

Conversation

hkershaw-brown
Copy link
Member

Description:

Puts the %copies array into the mpi window for get_state (forward operator)
Removes the copy of the %copies(1:real_ens_members, :) to the window.
This reduces the per-code memory usage (removes a %copies(1:real_ens_members, 1:my_num_vars)
~1/2 for the state per memory core usage.

The offset is calculated from %num_copies, the get_state is only the real ensemble members

ink

I think this is a good time to remove the cray window module.

Fixes issue

fixes #718

Types of changes

  • 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 not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

Tests

There is a developer test included. Note this needs fortran-testanything code to run.
Bitwase lorenz_96 mpi, mpif08, nompi

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Update conf.py

Checklist for release

  • Merge into main
  • Create release from the main branch with appropriate tag
  • Delete feature-branch

Testing Datasets

  • Dataset needed for testing available upon request
  • Dataset download instructions included
  • No dataset needed

using external fortran-testanything
model_mod.f90 is in the work directory. This is a lazy move to avoid fiddling with EXTRA in quickbuild.sh

for issue #718
mpi version with all of %copies in the window
mpif08 version with all of %copies in the window
null_mpi_utilities_mod.f90 version with matchin arguments to the mpi version (does nothing)

fixes #718
hkershaw-brown and others added 2 commits September 11, 2024 10:00
The cray window mod was originally added because not all compilers/
mpi implemenations (mpi 2.0) supported mpi windows without using
cray pointers. The cray_win_mod has not been compiled into dart for
several years, and support for mpi windows has improved (mpi 3.0)
@hkershaw-brown
Copy link
Member Author

@hkershaw-brown check PMO

Copy link
Contributor

@mjs2369 mjs2369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these identical comments -

!>@todo should this be in the window_mod? you will have to change in both cray
!> and non cray versions

!>@todo should this be in the window_mod? you will have to change in both cray
!> and non cray versions

@mjs2369
Copy link
Contributor

mjs2369 commented Oct 30, 2024

Do we want to rename the window mods to not mention cray?

no_cray_win_mod.f90 -> win_mod.f90
no_cray_winf08_mod.f90 -> winf08_mod.f90

! These routines are passed through from default_model_mod.
! To write model specific versions of these routines
! remove the routine from this use statement and add your code to
! this the file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the template model_mod, it it just in there to compile the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change the quickbuild.sh to get the template model mod rather than a copying (lazy see commit 9d0f897)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter if we aren't going to put the tests in the repo, but if we do I think we should use the template

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used template, added fortran-test anything to developer tests.

@mjs2369
Copy link
Contributor

mjs2369 commented Oct 30, 2024

In buildfunctions.sh and buildconvfunctions.sh, you can remove all mentions of windowsrc. It's not needed anymore.

I just pushed up my commits from when I removed the cray window a while back if you want to take a look at these or cherry pick and commits (on branch remove_craywin). It has windowsrc already taken out of the code. It also has the window files already renamed and updates buildfunctions.sh and buildconvfunctions.sh accordingly

@hkershaw-brown
Copy link
Member Author

In buildfunctions.sh and buildconvfunctions.sh, you can remove all mentions of windowsrc. It's not needed anymore.

I just pushed up my commits from when I removed the cray window a while back if you want to take a look at these or cherry pick and commits (on branch remove_craywin). It has windowsrc already taken out of the code. It also has the window files already renamed and updates buildfunctions.sh and buildconvfunctions.sh accordingly

Good point. I shall grab your changes from remove_craywin.
Since we only have null/mpi/mpif08 windows we don't have to have a separate window file (cray vs not cray). I quite like the window being in a separate file rather than adding more stuff to {null|mpi}_utilties_mod.f90

@hkershaw-brown
Copy link
Member Author

In buildfunctions.sh and buildconvfunctions.sh, you can remove all mentions of windowsrc. It's not needed anymore.

I just pushed up my commits from when I removed the cray window a while back if you want to take a look at these or cherry pick and commits (on branch remove_craywin). It has windowsrc already taken out of the code. It also has the window files already renamed and updates buildfunctions.sh and buildconvfunctions.sh accordingly

cherry-picked your commits onto this branch

from dennisdjensen
https://github.com/dennisdjensen/fortran-testanything
license included in this commit.
renamed test.f08 to test.f90 because *.f90 is what we have mkmf looking for

updated buildfunctions to include the fortran-testanything code for developer tests only
removed the local threed_model_mod.f90 (using the template model path like other developer tests)
@hkershaw-brown
Copy link
Member Author

Remove these identical comments -

!>@todo should this be in the window_mod? you will have to change in both cray
!> and non cray versions

!>@todo should this be in the window_mod? you will have to change in both cray
!> and non cray versions

done

Copy link
Contributor

@mjs2369 mjs2369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good, developer test working well.

@hkershaw-brown hkershaw-brown added the release! bundle with next release label Nov 6, 2024
adding missing v to tags in changelog

attribution to dennisdjensen for fortran-testanything
@hkershaw-brown hkershaw-brown merged commit 9eb01c7 into main Nov 7, 2024
4 checks passed
@hkershaw-brown hkershaw-brown deleted the mpi-win branch November 7, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release! bundle with next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Reduce memory usage for forward operators MPI window
2 participants