Bug: Fix success_criteria to track when clause precedence#1653
Bug: Fix success_criteria to track when clause precedence#1653tteggelit wants to merge 10 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how success criteria are stored and resolved in Ramble. Success criteria are now grouped by their conditional 'when' sets (using frozenset) to handle conflicting conditions and prevent duplicate definitions. The WRF application's success criteria have been cleaned up, and the base class analysis logic has been updated to handle the new nested structure. The review feedback suggests simplifying the resolved_criteria dictionary to a set, as its values are never read and it is only used for membership testing.
|
/gcbrun |
Ramble Performance Test MetricsResults produced with commit: b34e11a
|
linsword13
left a comment
linsword13
left a comment
There was a problem hiding this comment.
Would it be useful to add in some unit tests for the new/correct behavior?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1653 +/- ##
===========================================
+ Coverage 93.30% 93.32% +0.02%
===========================================
Files 352 353 +1
Lines 34223 34311 +88
===========================================
+ Hits 31931 32020 +89
+ Misses 2292 2291 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…t/ramble into success_criteria_precedence_bug
I added some unit tests to cover various precedence situations. |
|
@linsword13 I think with this latest push, it should be ready for a re-review. |
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates Ramble's success criteria handling to group criteria by their 'when' conditions, allowing the detection of conflicts when the same success criteria is defined multiple times under conflicting satisfied conditions. It also adds comprehensive tests and mock applications to verify precedence, inheritance, and conflict behavior. The review feedback suggests removing an unused loop variable 'prec' in 'base_class.py' and eliminating redundant loops in the test file that repeatedly open and assert against the same results file.
|
@linsword13 Implemented the Gemini code review suggestions, so I'll need one more re-review kicked off. |
|
/gemini review |
|
/gcbrun |
There was a problem hiding this comment.
Code Review
This pull request refactors how success criteria are stored and resolved in Ramble to handle conflicts, inheritance, and mutually exclusive conditions correctly. Specifically, success criteria are now grouped by their when conditions (as a frozenset), and the analysis resolution logic has been updated to detect and error out on conflicting satisfied conditions, while respecting inheritance precedence. Additionally, a new set of unit tests and mock applications have been introduced to verify these behaviors. I have no feedback to provide as there are no review comments to evaluate.
|
/gcbrun |
|
@linsword13 I think this is now ready for final review. |
| packages: {} | ||
| environments: {} | ||
| """ | ||
| with ramble.workspace.create(workspace_name) as ws: |
There was a problem hiding this comment.
nit: There's a pytest fixture make_workspace_from_config that may simplify some of these setup.
There was a problem hiding this comment.
The unit test I cribbed off of did it this way, but I can update it to use make_worksapce_from_config if that is the preferred method now.
There was a problem hiding this comment.
OK. Just committed update to switch to make_workspace_from_config
| existing_when_set = obj_satisfied_criteria[name][0] | ||
| logger.die( | ||
| f"Success criteria '{name}' in object '{obj_inst.name}' is defined multiple times " | ||
| f"under conflicting satisfied 'when' conditions:\n" |
There was a problem hiding this comment.
Just curious: does that mean this will not tolerate duplicate when?
There was a problem hiding this comment.
Without the specificity functionality we talked about, if there are multiple satisfying when conditions at the same precedence level, it will trigger an error. This seems to be the current way of handling this. Allowing 'when' specificity would change this and only trigger an error if there were multiple satisfies at the same specificity.
| mode="application_function", | ||
| owning_object=self, | ||
| ) | ||
| if "_application_function" not in resolved_criteria: |
There was a problem hiding this comment.
There may be no current code path that defines an _application_function scoped success_criteria currently, but I added this in defense in case there is. I don't believe add_criteria() does any kind of deduplication so if one already existed for some reason, there would be duplicate criteria added then.
2 participants
Currently,
success_criteriaallows for scoping viawhenclauses, but definitions of these criteria are not properly tracked in thesuccess_criteriadict. In cases where multiplewhenclauses are satisfied and define the same criteria name, this leads to the last defined criteria winning. This fix properly tracks satisfyingwhenconditions selecting the highest precedence criteria.In the case where multiple
whenconditions are satisfied at the same precedence, ramble will now throw an error. Because of this, the WRF application was also fixed as it defined both an unscoped "Complete" criteria and one for specific WRF versions. The versioned definitions are mutually exclusive, but the unscoped criteria would always be satisfied resulting in a conflict between it and the satisfied versioned criteria.