Confusing indentation for bl_mynn_closure in README.namelist#2218
Closed
kkeene44 wants to merge 4 commits intowrf-model:release-v4.7.1from
Closed
Confusing indentation for bl_mynn_closure in README.namelist#2218kkeene44 wants to merge 4 commits intowrf-model:release-v4.7.1from
kkeene44 wants to merge 4 commits intowrf-model:release-v4.7.1from
Conversation
wrf-model#2185) TYPE: bug fix KEYWORDS: WRF-Chem, dry air density SOURCE: NOAA GSL, Alexander Ukhov (KAUST) DESCRIPTION OF CHANGES: Problem: It was found that dry air density was miscalculated. Solution: Removed unnecessary factor. Simulations before and after did not show any significant difference, as expected. LIST OF MODIFIED FILES: M chem/module_chem_utilities.F TESTS CONDUCTED: The Jenkins tests are all passing. RELEASE NOTE: Fixed calculation of dry air density in module_chem_utilities.F. The bug had a very minor effect.
…-model#2143) TYPE: enhancement KEYWORDS: testing, devops, github, workflow SOURCE: internal DESCRIPTION OF CHANGES: Problem: The CI/CD testing framework using github actions is set to trigger on PR label events. However, labels are not just used for testing and will trigger the workflow. While the workflow does have checks in place to skip any labels that aren't meant to trigger testing, this skipped workflow status will override any previous actual test status. This can be confusing if two labels are applied at the same time, one being a test label, and only one status appears showing a skipped workflow. Unique naming of workflow runs does not mitigate this problem as the posted status is tied to the workflow internal id and not the run name. Solution: Convert the main workflow that hosts the test sets into a dispatch workflow, meaning it must manually be triggered. This has the intended effect of decoupling the workflow id from any PR and will not normally show up as a status at the bottom of PRs solving the issue of independent labels overriding each other. To solve the workflow no longer appearing within PR statuses, the github REST API is used to create a commit status pointing to its respective workflow run via `target_url` along with the current state of the job. As the main workflow must manually be triggered, a new entry point proxy workflow is used to filter test labels and request a test run if needed. This paradigm allows events to be triggered within a PR context, simplifying gathering the data necessary to run the correct PR branch. Furthermore, the entry point will still suffer the initial problem of status override on multiple labels, but this should be acceptable as actual test labels will create their own commit statuses once queued. The dispatch workflow is unable to be run within the context of PR merge refs, nor the head of the branch from a fork (as that would run the workflow in _that_ fork). Thus, the dispatch workflow is run using the base ref of the PR if from a fork, or the head ref *ONLY IF* originating from the parent repo of the workflow. This means testing of the immediate changes to the workflow can only be observed within the PR of an internal repo branch, limiting development slightly. The benefits, however, are a cleaned up status reporting AND increased security as no runner code that isn't already within a branch of the repository will be executed. One should still ensure the underlying tests are okay to run TESTS CONDUCTED: 1. Testing was done in independent fork to demonstrate initial issue and feasibility of this solution.
Contributor
|
Thank you, Kelly! |
Collaborator
|
@kkeene44 I tried to change this to go into v4.7.1 but it looks like there are a few things it would pull in from develop. I'll leave it up to you if you want to shift this into the bug fix branch |
dudhia
requested changes
May 7, 2025
Collaborator
dudhia
left a comment
dudhia
left a comment
There was a problem hiding this comment.
This whole section should be moved after all the PBL options with the other bl_mynn options.
weiwangncar
previously approved these changes
May 29, 2025
weiwangncar
reviewed
May 30, 2025
| bl_mynn_closure = 2.5: level 2.5 | ||
| 2.6: level 2.6 | ||
| 3.0: level 3.0 | ||
| = 7, ACM2 (Pleim) PBL |
Collaborator
There was a problem hiding this comment.
@kkeene44 Can we move these three lines to after tke_budget where other bl_mynn_* options are listed?
Collaborator
|
This PR is replaced by PR-2230 and closed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TYPE: text only
KEYWORDS: README.namelist, bl_mynn_closure, bl_pbl_physics, indent
SOURCE: internal
DESCRIPTION OF CHANGES:
Problem:
In README.namelist, the options for bl_mynn_closure were indented in a way that made it seem they were part of the above bl_pbl_physics options and descriptions.
Solution:
Corrected the indentation so that bl_mynn_closure is now its own entry.
LIST OF MODIFIED FILES:
M run/README.namelist
TESTS CONDUCTED: