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

Adds extra metadata to fields about their origin #241

Merged
merged 19 commits into from
Dec 10, 2024

Conversation

uturuncoglu
Copy link
Contributor

This PR aims to enhance NUOPC layer by adding extra metadata to the import and export states and also fields in those states about their origin. By this way, the acceptor component could query the metadata to see the origin of the fields like following,

       ! Get attribute from field
       call NUOPC_GetAttribute(field, name="ProviderCompName", value=cvalue, &
         isPresent=isPresent, isSet=isSet, rc=rc)
       if (ChkErr(rc,__LINE__,u_FILE_u)) return

@uturuncoglu uturuncoglu added the feature/enhancement New feature or request label May 2, 2024
@uturuncoglu
Copy link
Contributor Author

@theurich @oehmke @danrosen25 I could able to query the fields in acceptor side and it looks like following,

 Field, ProviderCompName = ocean_fraction ATM
 Field, ProviderCompName = ocean_mask OCN

At this point I am aiming to create namespaces and nested states in the generic component side based on the information provided by the ProviderCompName metadata and plan to advertise fields to their own nested states. At this point, I am not sure how this will work. The generic component import state is a minor of the attached component export states and all the information is coming to import state in ModifyAdvertised. Is it possible to create namespaces and nested states after this point. If so, which phase need to be used and which phase will define namespaces and nested states. This is done in the Advertise for the mediator but I am not sure about the eugenic component. It is act like a mediator but little bit different.

@uturuncoglu
Copy link
Contributor Author

@theurich @oehmke @danrosen25 BTW, I am not sure this is the right approach to make provider name information available to other components. Anyway, let me know what you think.

@uturuncoglu
Copy link
Contributor Author

@theurich I add a new option transferAllNested to enable having nested states in the mirrored import and export states. I tested at least mirroring import states and it seems it is working. I think this also needs some entry also to documentation. If you don't ming could you point me the right place to add it. I think transferAllNested and also new metadata in field level CompName (provider name indicated in the run sequence) needs to be added. I'll keep testing and making brach synced with develop. When you have please check the implementation and let me know if something missing or logically wrong.

@uturuncoglu
Copy link
Contributor Author

@theurich I added extra documentation. Please review it when you have time. I am also working on NUOPC app prototype to test new feature. Once it is ready, I'll open another PR in there and linked to here.

@theurich
Copy link
Member

@uturuncoglu sounds good. It is on my todo list. :-)

@uturuncoglu
Copy link
Contributor Author

@theurich BTW, we could extend the existing AtmOcnMirrorFields app but create new one from scratch. My impression is the creating a new one just dedicated to new option by simplifying AtmOcnMirrorFields would be better but just let me know if you have any preference.

@theurich
Copy link
Member

@theurich BTW, we could extend the existing AtmOcnMirrorFields app but create new one from scratch. My impression is the creating a new one just dedicated to new option by simplifying AtmOcnMirrorFields would be better but just let me know if you have any preference.

Yes, a dedicated proto is my preference here, too!

@uturuncoglu
Copy link
Contributor Author

@theurich I wonder if it is possible to set following attributes through use of NUOPC_SetAttribute. In the new NUOPC app prototype, I need to set them in ModifyAdvertised phase but actually, they are set using NUOPC_Advertise() call in the existing prototype.

SharePolicyGeomObject="share", &
      SharePolicyField="share", &

So, i am trying to construct the atmospheric component by querying nested states and without setting those sharing is not working properly and code hangs at some point. Anyway, let me know what you think. I could push current version to a branch if you want to see.

@uturuncoglu
Copy link
Contributor Author

@theurich Okay. I found it. No worries.

@uturuncoglu
Copy link
Contributor Author

New draft app prototype PR can be found in here esmf-org/nuopc-app-prototypes#7

@theurich
Copy link
Member

theurich commented Dec 5, 2024

@uturuncoglu Please merge develop into this branch. Then kick off the Development Tests action for this branch. Once that passes, please mark PR "ready for review". Thanks.

@uturuncoglu
Copy link
Contributor Author

@theurich Okay. Testing soon.

@danrosen25
Copy link
Member

@uturuncoglu @theurich
Development Tests don't include NUOPC System Tests (a.k.a. https://github.com/esmf-org/nuopc-app-prototypes). So you'll have to kick them off externally. I'd like to include NUOPC System Tests in the Development Tests but I haven't decided on the design due to the non-synchronized development heads of the two repositories. I'm thinking that in the manually executed workflow there will be an option for running NUOPC System Tests and it will let you choose the branch.

@theurich
Copy link
Member

theurich commented Dec 5, 2024

@danrosen25 I like the idea of being able to select the branch for NUOPC app protos repo once this feature is integrated into the Development Tests action.

@uturuncoglu
Copy link
Contributor Author

@theurich I have merged develop to both ESMF and NUOPC app port branches. I also install the updated version of ESMF in my end. it seems that HierarchyProto is still failing with the recent fix. Let me know what you think? BTW, is there any way that I could double check I am using updated installation. In the PET log it says like v8.8.0b06-11-g1e3d81d391 but I am not sure what does it mean. If you want I could also try with fresh install.

@uturuncoglu
Copy link
Contributor Author

uturuncoglu commented Dec 5, 2024

@theurich It seems that ESMF_CompTunnelUTest.F90 test is failing like

FAIL  Check time delay on non-blocking Component D Finalize/wait, ESMF_CompTunnelUTest.F90, line 1283:  Incorrect time delay

and I have

--------------------------------------------------------------------------
A system call failed during shared memory initialization that should
not have.  It is likely that your MPI job will now either abort or
experience performance degradation.

  Local host:  Mac-1733424547768.local
  System call: unlink(2) /var/folders/0w/4z5l9vds32nbkz7l22n8j6s80000gn/T//ompi.Mac-1733424547768.501/pid.47393/1/vader_segment.Mac-1733424547768.501.e49e0001.6
  Error:       No such file or directory (errno 2)

If I remember this was an issue before and not related with the PR. It is failing on just one particular combo.

@theurich
Copy link
Member

theurich commented Dec 5, 2024

Yes, I saw that in my "Development Tests" action, too, yesterday. Strange thing is that it had not been happening on develop for a while. But certainly not related to your PR.

@danrosen25
Copy link
Member

It's the same issue I tried to fix by downgrading the OpenMPI Library. The delay time is < 3 seconds, which is impossible. The precTime value 2.8999999999612669E-005 should be catching any rounding errors but the error is much much larger than the precision of MPI time. It's hard to tell what's happening, I investigated months ago and came to the conclusion that it is probably due to the OpenMPI library or the GitHub runner (virtualization). I haven't thought of an alternative way to calculate ESMF_VMWtime that would fix this issue.

PET4  FAIL  Check time delay on non-blocking Component D Finalize/wait, ESMF_CompTunnelUTest.F90, line 1283:  Incorrect time delay
PET4  delayTime (wait) =    2.9986590000000000

@danrosen25
Copy link
Member

@theurich
I found some information on an issue within runner-images. I'm trying a fix right now and if it works I'll open a pull request.
actions/runner-images#820

Fix Branch: https://github.com/danrosen25/esmf/tree/refs/heads/fix/devtest_macostime

@danrosen25
Copy link
Member

Yes, I saw that in my "Development Tests" action, too, yesterday. Strange thing is that it had not been happening on develop for a while. But certainly not related to your PR.

@uturuncoglu @theurich - This should be fixed now

@theurich
Copy link
Member

theurich commented Dec 5, 2024

Yes, I saw that in my "Development Tests" action, too, yesterday. Strange thing is that it had not been happening on develop for a while. But certainly not related to your PR.

@uturuncoglu @theurich - This should be fixed now

@danrosen25 that's awesome! Thank you!

@uturuncoglu uturuncoglu marked this pull request as ready for review December 8, 2024 03:05
@uturuncoglu
Copy link
Contributor Author

@theurich Okay. I merged your PR and make this to ready for review.

@theurich
Copy link
Member

theurich commented Dec 9, 2024

Moving and continuing conversation from #328:

@theurich That sounds good to me. Do you also plan to change the structure of the metadata (i.e. atm-importstate-namespace:ocn). The intention to have the metadata to query provider component quickly. In this way, user need to parse the string to get the information. It is fine for me but just wondering about your feeling about it.

User code should access the "Namespace" attribute, not the nested state name. The "Namespace" attribute is guaranteed to be identical to the component label of the user provided run sequence. So NUOPC internal name mangling for state names, etc., does not affect the "Namespace" attribute. Does that make sense?

@uturuncoglu
Copy link
Contributor Author

@theurich Okay. That makes sense. Please go ahead and merge it if you have all the changes (incl. doc). Let me know if you need anything from my end.

@theurich
Copy link
Member

theurich commented Dec 9, 2024

Sounds good. It will take me a little bit. I will push them and let you know when I am finished with it. Also will touch your prototype app code at the same time.

FieldTransferPolicy attribute. Update documentation.
@theurich theurich requested a review from danrosen25 December 9, 2024 20:09
@theurich
Copy link
Member

theurich commented Dec 9, 2024

@uturuncoglu
Copy link
Contributor Author

@theurich I have just tested your changes with my application and it is working as expected after some minor modification. So, I think this is good to go.

@uturuncoglu
Copy link
Contributor Author

@uturuncoglu and @danrosen25 the updated mirror section in the documentation can be reviewed at https://earthsystemmodeling.org/docs/nightly/feature/provider_metadata/NUOPC_refdoc/node3.html#SECTION00034900000000000000

Documentation also looks good. Thanks for all your help.

@theurich
Copy link
Member

@uturuncoglu That sounds good. I'll go ahead and merge the PR.

@theurich theurich merged commit 523ca69 into develop Dec 10, 2024
6 checks passed
@theurich theurich deleted the feature/provider_metadata branch December 10, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants