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

Suite name guidelines #72

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

mkavulich
Copy link
Collaborator

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

@climbfuji
Copy link
Collaborator

Will these new suite names work out of the box with prebuild and capgen? I actually don't remember.

Copy link
Collaborator

@climbfuji climbfuji left a 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!

Copy link
Collaborator

@climbfuji climbfuji left a 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?

@mkavulich
Copy link
Collaborator Author

@climbfuji to answer your first question, there doesn't appear to be anything in capgen that enforces certain naming conventions (aside from an xml suffix), unlike prebuild which contains SUITE_DEFINITION_FILENAME_PATTERN regex (which will be updated to remove the "suite" prefix requirement). In fact, it looks like the current capgen test suites already conform to this new standard (suitename.xml rather than suite_suitename.xml)

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

@mkavulich
Copy link
Collaborator Author

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.

Copy link
Collaborator

@climbfuji climbfuji left a 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?

@mkavulich
Copy link
Collaborator Author

mkavulich commented May 22, 2024

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.

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