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

fixed profiler dependency on new gftl-v2 #1335

Merged
merged 6 commits into from
Feb 7, 2022

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Feb 4, 2022

Fix profiler dependency on gftl-v2

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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 change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

@weiyuan-jiang weiyuan-jiang requested review from a team as code owners February 4, 2022 14:49
@weiyuan-jiang weiyuan-jiang added 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) bugfix labels Feb 4, 2022
@weiyuan-jiang
Copy link
Contributor Author

This PR addresses issue #1334

@mathomp4 mathomp4 added the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Feb 4, 2022
@mathomp4
Copy link
Member

mathomp4 commented Feb 4, 2022

Blocking for just a second so I can fix up the branch for release.

@mathomp4 mathomp4 added 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs and removed 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs labels Feb 4, 2022
@mathomp4
Copy link
Member

mathomp4 commented Feb 4, 2022

Actually, let me keep blocked as I run a GEOSgcm test.

@mathomp4
Copy link
Member

mathomp4 commented Feb 4, 2022

Zero-diff and builds.

@mathomp4 mathomp4 removed the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Feb 4, 2022
@weiyuan-jiang weiyuan-jiang added the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Feb 4, 2022
@weiyuan-jiang
Copy link
Contributor Author

Add label DNA back. I want to understand what happens here

@weiyuan-jiang
Copy link
Contributor Author

I guess I figure out what happened here. The include path can be inherited from dependence. Since profiler depends on mapl.shared, it inherits its include path. Mapl.shared depends on pflogger which has all gFTL's paths. In our GEOSgcm, we build and use pFlogger but in UFS, only pFlogger_stub is used. That is why in UFS, v2 path is missing. This PR should work for UFS, but we may add fake dependence to pfloger_stub to make it really mimic pFlogger.

@tclune
Copy link
Collaborator

tclune commented Feb 7, 2022

@weiyuan-jiang Well done!

And this speaks directly to why it is so important for us to list all the explicit dependencies of each layer. Otherwise we get surprising results when unrelated layers are changed elsewhere in the model.

@mathomp4
Copy link
Member

mathomp4 commented Feb 7, 2022

I guess I figure out what happened here. The include path can be inherited from dependence. Since profiler depends on mapl.shared, it inherits its include path. Mapl.shared depends on pflogger which has all gFTL's paths. In our GEOSgcm, we build and use pFlogger but in UFS, only pFlogger_stub is used. That is why in UFS, v2 path is missing. This PR should work for UFS, but we may add fake dependence to pfloger_stub to make it really mimic pFlogger.

Dang. Good job figuring this out. CMake is...fun this way!. The question for @tclune is: should @weiyuan-jiang add the fake dependence in pflogger_stub?

@mathomp4
Copy link
Member

mathomp4 commented Feb 7, 2022

@weiyuan-jiang Okay. I talked with @tclune. I think what you should do is this. In both generic/CMakeLists.txt and in profiler/CMakeLists.txt, we should have:

GFTL::gftl-v1 GFTL::gftl-v2

The reason is that both are currently using both! You can see all the v2 uses:

❯ rg 'template.inc'
generic/VarConnVector.F90
8:#include "vector/template.inc"

generic/VarSpecVector.F90
8:#include "vector/template.inc"

profiler/MeterNodeStack.F90
9:#include "vector/template.inc"

but of course they use v1 in many more places!

@weiyuan-jiang weiyuan-jiang removed the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Feb 7, 2022
@weiyuan-jiang
Copy link
Contributor Author

@weiyuan-jiang Okay. I talked with @tclune. I think what you should do is this. In both generic/CMakeLists.txt and in profiler/CMakeLists.txt, we should have:

GFTL::gftl-v1 GFTL::gftl-v2

The reason is that both are currently using both! You can see all the v2 uses:

❯ rg 'template.inc'
generic/VarConnVector.F90
8:#include "vector/template.inc"

generic/VarSpecVector.F90
8:#include "vector/template.inc"

profiler/MeterNodeStack.F90
9:#include "vector/template.inc"

but of course they use v1 in many more places!

Changed

@mathomp4
Copy link
Member

mathomp4 commented Feb 7, 2022

Thanks. I'll do a build-and-run test to be sure all is well.

@mathomp4 mathomp4 added 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs and removed 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs labels Feb 7, 2022
@mathomp4
Copy link
Member

mathomp4 commented Feb 7, 2022

Works for me. Approving.

mathomp4
mathomp4 previously approved these changes Feb 7, 2022
@mathomp4 mathomp4 requested a review from tclune February 7, 2022 16:33
@tclune
Copy link
Collaborator

tclune commented Feb 7, 2022

GFTL::gftl and GFTL::gftl-v1 are the same thing. So one should not specify them both.

Someday GFTL::gftl will be an alias for gftl-v2. It sort of provides a future-safe way of upgrading, except that the Fortran files still need to change for it to help. Probably a waste of effort on my part.

@mathomp4 mathomp4 self-requested a review February 7, 2022 16:59
@mathomp4 mathomp4 merged commit 73a2cfa into main Feb 7, 2022
@mathomp4 mathomp4 linked an issue Feb 7, 2022 that may be closed by this pull request
@mathomp4 mathomp4 deleted the hotfix/wjiang/profiler_gftlv2 branch February 7, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) 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.

Missing gFTL v2 target for MAPL.profiler prevents MAPL from building
3 participants