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

Add LD_PRELOAD for Coupled Model #341

Merged
merged 4 commits into from
Aug 4, 2022
Merged

Conversation

mathomp4
Copy link
Member

@mathomp4 mathomp4 commented Jul 27, 2022

Recent testing with mom5 by @yvikhlya and @sanAkel (see GEOS-ESM/GEOSgcm#352 and GEOS-ESM/GEOSgcm#430) in GEOSgcm has shown a need to bring back the old LD_PRELOAD mechanism that was removed in #204. So this PR brings back LD_PRELOAD but only in the case of MOM5

Obviously not ideal, but until this oddity can be examined by @tclune and @weiyuan-jiang (and maybe @atrayano), we bring back LD_PRELOAD ONLY when running MOM5 as runs of MOM6 don't seem to care!

As @yvikhlya pointed out below, MOM6 might only work due to luck. So we preload both.

@mathomp4 mathomp4 added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Jul 27, 2022
@mathomp4 mathomp4 self-assigned this Jul 27, 2022
@yvikhlya
Copy link

@mathomp4 I thinks both MOM5 and MOM6 need explicit LD_PRELOAD. MOM6 does not care now because it luckily goes first in load list (check ldd GEOSgcm.x), but if somebody will change something in cmake for example which will cause libmom6.so to go behind libmom5.so, we will scratch our heads again, why MOM6 does not work

@mathomp4
Copy link
Member Author

@mathomp4 I thinks both MOM5 and MOM6 need explicit LD_PRELOAD. MOM6 does not care now because it luckily goes first in load list (check ldd GEOSgcm.x), but if somebody will change something in cmake for example which will cause libmom6.so to go behind libmom5.so, we will scratch our heads again, why MOM6 does not work

Hmm. Well, I can add that if you like. Give me a second...

@mathomp4 mathomp4 changed the title Add LD_PRELOAD for mom5 Add LD_PRELOAD for Coupled Model Jul 27, 2022
@mathomp4 mathomp4 requested review from yvikhlya and sanAkel July 27, 2022 18:59
@mathomp4
Copy link
Member Author

@yvikhlya Okay, this should now preload both.

yvikhlya
yvikhlya previously approved these changes Jul 27, 2022
@mathomp4 mathomp4 changed the title Add LD_PRELOAD for Coupled Model WIP: Add LD_PRELOAD for Coupled Model Jul 27, 2022
@mathomp4
Copy link
Member Author

Note: I probably need to generalize this for macOS since it is libmom6.dylib on that. I'll need to figure the best way to do this.

@mathomp4 mathomp4 changed the title WIP: Add LD_PRELOAD for Coupled Model Add LD_PRELOAD for Coupled Model Jul 28, 2022
@mathomp4 mathomp4 marked this pull request as ready for review July 28, 2022 19:08
@mathomp4 mathomp4 requested a review from a team as a code owner July 28, 2022 19:08
@mathomp4
Copy link
Member Author

I've pushed my generalized updates. Honestly, I think LD_PRELOAD might be ignored on macOS, not sure as it seemed to work with both .so and .dylib and the first obviously doesn't exist. But, well, I figure we should at least make it look right on macOS. 😄

Copy link
Collaborator

@sanAkel sanAkel left a comment

Choose a reason for hiding this comment

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

Seems to work for MOM6.

I have no working restarts at 1-deg for MOM5, but by the history of this issue and given the lines of changes, I am confident that it will work for MOM5 as well.

For the record, this PR
fixes: GEOS-ESM/GEOSgcm#430
and
resolves: GEOS-ESM/GEOSgcm#352

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