-
Notifications
You must be signed in to change notification settings - Fork 33
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
Suite name guidelines #72
base: main
Are you sure you want to change the base?
Conversation
Will these new suite names work out of the box with prebuild and capgen? I actually don't remember. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through your presentation. At first glance, I am not in favor of using completely meaningless names for the suites. As a developer who makes regularly large changes and then needs to go and fix hundreds of tests, it is going to be more effort to figure out which regression test uses which suite.
But I understand that the decision to use bird names is UFS specific. The only part of this PR that will apply to everyone (if changes to the framework are needed) is the removal of the suite_
prefix. In addition, what should apply to everyone is the request to add a comment in the suite to explain its contents.
Did I understand that correctly? If yes, then this PR in its present does not belong to this repository. ccpp-doc
is for all users and developers of CCPP, not just the UFS. Please remove the UFS specific parts and add them elsewhere to UFS-related documentation. Only the part that applies to all models should be submitted to ccpp-doc
.
Lastly, where and when was this discussed? I missed the last ccpp framework dev meeting, but from the notes it wasn't discussed there it seems.
Thanks!
…ry to avoid UFS-centrism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Can you clarify if changes are needed to prebuild or capgen?
@climbfuji to answer your first question, there doesn't appear to be anything in capgen that enforces certain naming conventions (aside from an Regarding the separation of concerns (host vs. CCPP), admittedly, suite files fall in a weird in-between state of governance between the host and CCPP, as the suite files reside in a host's repository, but the format and, for the most part, contents (and even some aspects of file names) are dictated by the CCPP framework and physics. I will not assert that this initial draft is perfect, and I've updated the wording based on your initial concerns, especially to try to dampen any "UFS-centrism" bias. Let me know what you think, or if you have any specific suggestions. Sorry you were caught off-guard by this, I did my best to cast a wide net when workshopping this proposal; outside of the extensive DTC discussions we've discussed this at several CCPP code management meetings, and I gave a brief presentation at the UFS App coordination meeting three weeks ago. I probably should have added it as a line item to a framework meeting, but I thought if discussion was needed it could happen once a framework PR was open (I will open an issue today). |
Also, I will open an issue in the UFS weather model repository for this upcoming change, along with a draft PR. We should discuss ways to mitigate any issues related to regression tests or other modifications that may be more difficult with "non-descriptive" suite names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. Approved, but maybe it's best to wait until the ccpp-framework PR for prebuild is merged?
Yes, I mainly opened this PR to have somewhere to point for documentation when opening other issues and PRs. This should not be merged until all discussion is concluded and things are (at least close to) approved. I'll convert this to a draft to emphasize that it is not ready for merge. |
Per discussions that have occurred over the past weeks/months, suite name guidelines are being added to the Tech Doc. See this presentation for more details.
The built documentation can be found here: https://mike-k-tech-doc.readthedocs.io/en/latest/ConstructingSuite.html#suite-definition-file