Update Apps to work with purge of intermediate configuration object api on lfric core#208
Update Apps to work with purge of intermediate configuration object api on lfric core#208mo-rickywong wants to merge 8 commits intoMetOffice:mainfrom
Conversation
…amelist api from core
|
While this is a non-scientific change its still too large for only one review, please can you organise a SciTech reviewer as well? |
ss421
left a comment
There was a problem hiding this comment.
This change will need to have a linked PR in lfric-jedi. We can create a PR for this and test it. Since we have an issue with the change in MetOffice/lfric_core#175 I think its worth waiting for a fix before doing the test although it will likely just work with this change if I understand this correctly.
Ive added this PR to the list or required updates documented in: https://github.com/JCSDA-internal/lfric-jedi/pull/1206
@DanStoneMO - could you take a look at this please? Thanks.
The lfric-jedi link is not reachable, can you reference the linked PR when available. |
|
From the lfric2lfric perspective, everything looks in order when looking at the code in lfric_core. I run the lfric2lfric checks myself successfully. The only comment I have is that, in the follow-up ticket, the lfric2lfric init_mesh subroutine version should be updated so that it has the same parameters as the lfric_core init_mesh subroutine. Thank you, though strictly that comment is not relevant to this PR, but should be made on the follow-up PR |
ukmo-juan-castillo
left a comment
There was a problem hiding this comment.
From the lfric2lfric perspective, everything looks in order when looking at the code in lfric_core. I run the lfric2lfric checks myself successfully. The only comment I have is that, in the follow-up ticket, the lfric2lfric init_mesh subroutine version should be updated so that it has the same parameters as the lfric_core init_mesh subroutine.
Thank you, though strictly that comment is not relevant to this PR, but should be made on the follow-up PR
(@mo-rickywong )
@DanStoneMO has a branch but the tests are failing at runtime, see for example: ctest_jjtests_hofx_atms__spice_gnu_debug . I think this is the same issue we have already seen and was fixed via: MetOffice/lfric_core#264. It seems the Would it be possible to merge upto the head of main? |
iboutle
left a comment
There was a problem hiding this comment.
This change appears to affect far more than expected, i.e. there are KGO changes and all sorts going on here - I'm not sure if something has gone wrong with the branch or the diff, but is it possible to show a clean diff of the changes just in this PR?
This reverts commit 710ff9a.
|
Your CLA signature was found on the base branch, but you appear to have modified the CONTRIBUTORS.md file in this PR. Please do not edit the CONTRIBUTORS.md file. If you have already signed the CLA, revert changes to the file and your signature will be picked up. |
I tried to bring this up to date as JEDI what to take account of a the jedi fix. Though the merge changed lots of unexepcted stuff. I've reverted the change as bringing it up-to-date should change much |
thomasmelvin
left a comment
There was a problem hiding this comment.
These changes look fine but (without knowing the details) I am a bit confused by routines having both a config and configuration passed in which are very similar name so it's not really obvious what the difference is. Could one set be renamed? or will it be changed in the future?
PR Summary
This is a technical infrastructure change, it does not change science or the overall structure of applications. SciTech is N/A though code owners may wish to check their relevant code areas in case they have comments/objections.
Sci/Tech Reviewer: @allynt
Code Reviewer: @oakleybrunt
NOTE: @ukmo-juan-castillo added to review changes in a Code Owner capacity only
MetOffice/lfric_core#262 Removes instances of usage of the intermediate api to access namelist configurations from lfric_core. This however means that some routines in lfric_core have their argument lists changed, this may impact some applications in lfric_apps.
This PR only changes what is required to work with the linked MetOffice/lfric_core#262. Further PRs to remove usage of the Intermediate api from Lfric_apps will follow.
Linked PRs
Code Quality Checklist
Testing
trac.log
Testing was done with developer group and lfric_atm_extra as some iau is not exercised in the main developer group.
Test Suite Results - lfric_apps - AppsIntermediateApiPurge/run4
Suite Information: lfric_atm_extra group
Task Information
✅ succeeded tasks - 173
Test Suite Results - lfric_apps - AppsIntermediateApiPurge/run5
Suite Information: Developer group
Task Information
✅ succeeded tasks - 1106
Security Considerations
Performance Impact
N/A
AI Assistance and Attribution
No AI was used on this PR developerment
Documentation
N/A
PSyclone Approval
N/A
Sci/Tech Review
N/A : Code owners may wish to review / comment
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review