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

Focus on Namespace handling for targeted field mirror #328

Merged

Conversation

theurich
Copy link
Member

@theurich theurich commented Dec 6, 2024

Base the implementation of the new "transferAllAsNests" mirror option on the existing Namespace concept: Fields advertised via NUOPC_Connector into a State that has FieldTransferPolicy="transferAllAsNests" set becomes indistinguishable from manually advertised Fields under the target component Namespace.

Also ensure the implementation is compliant with the optimized ESMF_StateReconcile() behavior. Specifically do not depend on top level State Info being reconciled for empty States, or constituent fields (e.g. in nested State) being reconciled if nested State is object of the reconciling VM context.

@theurich theurich requested a review from uturuncoglu December 6, 2024 22:37
@theurich theurich self-assigned this Dec 6, 2024
@theurich
Copy link
Member Author

theurich commented Dec 6, 2024

With the new focus on Namespace handling, I am proposing the change the new option from "transferAllAsNests" to "transferAllWithNamespace". It is still true that this introduces Nests on the acceptor side, because NUOPC Namespaces are implemented via nested States, but the primary user facing concept is Namespace not Nest.
I did not make that change in name in this PR yet, because I wanted @uturuncoglu's new prototype case to work as it is for now, but I would like to discuss the suggested name change.

Copy link
Contributor

@uturuncoglu uturuncoglu left a comment

Choose a reason for hiding this comment

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

I am not very familiar to the namespace concept but this looks more clean solution. Maybe we could spend little bit time in the next core meeting and discuss about it little bit more. BTW, I just need to test this with the real application that I am working on. I think you are also proposing to change the name of the Transfer attribute. If so, we also need to update the documentation.

@uturuncoglu
Copy link
Contributor

@theurich let me know when you want me to merge this.

call NUOPC_SetAttribute(exportNestedState, "FieldTransferPolicy", "transferAll", rc=rc)
! set namespace on exportState, creating a nestedState on acceptor VM
call NUOPC_AddNamespace(exportState, namespace=trim(namespace), &
nestedStateName=trim(stateName)//"-namespace:"//trim(namespace), &
Copy link
Contributor

Choose a reason for hiding this comment

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

The nested state name is appearing in the NUOPC app prototype something like atm-importstate-namespace:ocn and this is aligned with the change in here. So, this is little bit different than the convention that I was using before. Are we plaining to go with this naming convention? If so, I need to couple of adjustment in my real application. Also, I think we need to document this convention in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's discuss this further next week (on a call, or under your PR). I am thinking your code should only be going off of the Namespace attribute, which is part of the NUOPC standard, and documented. The nested State name itself is a NUOPC implementation detail, and user code should not depend on it (I just made it something meaningful for the purpose of internal NUOPC development and debugging). But we can discuss this in more detail... I may be overlooking something.

@theurich
Copy link
Member Author

theurich commented Dec 7, 2024

@theurich let me know when you want me to merge this.

@uturuncoglu I think you could merge this into your PR branch. We can discuss more details under your PR.

@uturuncoglu uturuncoglu merged commit b4ab77b into feature/provider_metadata Dec 8, 2024
6 checks passed
@theurich theurich deleted the feature/provider_metadata-namespace branch December 9, 2024 17:07
@theurich
Copy link
Member Author

theurich commented Dec 9, 2024

@uturuncoglu if you and @danrosen25 are okay with it, I would like to rename the new "transferAllAsNests" to "transferAllWithNamespace", and also adjust the associated documentation. The implementation of course does not change. It's just that the user facing focus will be on "Namespace" which is an existing NUOPC concept. I am all set up locally to do this quickly. I would also adjust the prototype code accordingly.
I think once we have that in we can merge it into the develop branch even today.

@uturuncoglu
Copy link
Contributor

@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.

@theurich
Copy link
Member Author

theurich commented Dec 9, 2024

Sorry, I just noticed that I was writing these comments under the closed PR #328 of my changes into your PR #241. Meant to communicate under your open PR... I will move the conversation over there... #241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants