[End to End Testing] Implement must change. - #3182
Conversation
3f12c07 to
d2620ed
Compare
d2620ed to
8d885c9
Compare
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1134 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1134 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1134 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1134 |
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1133 |
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1133 |
ew3361zh
left a comment
ew3361zh
left a comment
There was a problem hiding this comment.
This seems close but is complex! I may need a walk through on this if possible. I left some notes on things I'm confused on or could use clarification with.
Also had trouble with multiple must-change variables across multiple domains working properly.
Here are the 2 I used:
"AnimalModuleReporter.report_animal_population_statistics.population_number_of_cows", "PurchasedFeedStorage.report_stored_purchased_feeds.stored_feed_202.balance_storage_levels"
I added them both to all must_change_variables jsons and then changed # of cows in the animal input thinking that would change the cow number and the feed balance since a different number of cows would be eating a different amount of feed.
I got:
Freestall
- Animal - must_change_violation for
population_number_of_cows - Feed - must_change_satisfied for
202.balance_storage_levels
Animals only:
- Animal - must_change_violation for
population_number_of_cows
Field and Feed:
- Feed - passing, nothing about 202 so I'm guessing it wasn't found to be matching. Maybe that should be reported?
Open Lot:
- Animal - must_change_violation for
population_number_of_cows - Feed - must_change_violation for
202.balance_storage_levels- this is the one that threw me because even though open lot uses a different feed input, it still uses 202 in rations so I would've expected it to pass.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "description": "Variables listed in 'must_change_variables' are expected to differ from the recorded expected results, e.g. because of a known model change whose new values are not yet known. During an end-to-end testing run, each listed variable must differ from its recorded expected value beyond the domain tolerance: a matching value is reported as a failure, and differences in listed variables are not. Variable names must exactly match keys in the 'expected_results' of this input set's e2e_json_*_filter.json files; the 'changed_variables' list in the end-to-end comparison results shows which variables currently differ. Managing this list is the user's responsibility and it defaults to empty: after updating the expected results (UPDATE_E2E_TEST_RESULTS), empty this list yourself, since the freshly recorded expected results already reflect the change and leftover entries will fail the next run.", | |||
There was a problem hiding this comment.
Do you think we could have this utilize regex-pattern-matching? I think of the variables that have the simulation day attached or a soil layer number attached and having the ability to match a series of variables across a range seems like a very likely scenario. If you like the idea, we could implement in a separate issue.
There was a problem hiding this comment.
Also what do you think about automatically clearing out these sections of must-change variables during an e2e expected results update?
There was a problem hiding this comment.
It's a great idea to have regex pattern matching. I will create an issue for it.
I'd prefer not to auto-clear. The must-change file records the SME's expectation, and I don't want the update task silently rewriting it. A leftover flag fails loudly on the next run with a self-explanatory message, while auto-clearing could silently discard a flag someone set for a change that hasn't landed yet.
| if not path.exists(): | ||
| om.add_error( | ||
| "End-to-end testing must-change configuration error", | ||
| f"Must-change variables file not found: {path}", | ||
| info_map, | ||
| ) | ||
| raise FileNotFoundError(f"E2E testing error: Must-change variables file not found: {path}") |
There was a problem hiding this comment.
Wonder if the validity of this path (and other paths referenced after e2e is complete) should be validated at the start somehow? Just thinking about how long e2e testing will take once the averaging results piece is factored in and these end-of-task validation errors seem brutal.
There was a problem hiding this comment.
Agreed, and all the inputs the comparison needs exist before the sim runs, so we can check them up front. This could also fit into a separate issue.
| "function": E2ETestResultsHandler.compare_actual_and_expected_test_results.__name__, | ||
| } | ||
| test_result_path_sets = E2ETestResultsHandler._get_test_result_paths(output_prefix) | ||
| must_change_variables = E2ETestResultsHandler._load_must_change_variables(test_result_path_sets) |
There was a problem hiding this comment.
Can you explain a little about this set-up?
If you gather the must_change_variables for all domains outside the test_result_path_sets loop but then check this entire set against each domain within that loop, does that mean the user should expect all those variables to change in whichever domain set of results they're found?
Are there circumstances where we'd expect a variable that's found in multiple domains to change in some simulations and not in others?
There was a problem hiding this comment.
It seems like this should maybe be within the loop and done at each iteration through for each individual path_set but it's a little confusing to me so maybe I have it wrong.
There was a problem hiding this comment.
Each task calls this once. All the domain entries of a set point at the same must-change file(freestall's Animal, CropAndSoil, Manure, and Feed entries all reference freestall_must_change_variables.json), so moving the load inside the loop would just read the same file four times.
On your other questions: within one input set every domain is just a filter over the same simulation, so a variable has one value per run, so if two domains both recorded it, they'd agree on whether it changed.
|
|
||
| diff = DeepDiff(expected_results, actual_results, ignore_order=True, verbose_level=2, significant_digits=3) | ||
| domain_must_change_variables = sorted(name for name in must_change_variables if name in expected_results) | ||
| matched_must_change_variables.update(domain_must_change_variables) |
There was a problem hiding this comment.
Same with this - should matched_must_change_variables be path_set specific (i.e. be created new within each loop through test_result_path_sets?
There was a problem hiding this comment.
It deliberately survives the whole loop. matched_must_change_variables is the memory of "which flagged names have been found in at least one domain so far".
| if is_difference_in_results: | ||
| om.add_error( | ||
| f"End-to-end testing failed for {domain}", | ||
| "Identified differences between actual and expected results.", | ||
| info_map, | ||
| ) | ||
| if must_change_violations: | ||
| om.add_error( | ||
| f"End-to-end testing failed for {domain}", | ||
| f"Must-change variables did not change: {sorted(must_change_violations)}", | ||
| info_map, | ||
| ) |
There was a problem hiding this comment.
Wondering if these could be distinguished in the e2e summary at the end of the e2e runs - e.g.
freestall_e2e results:
Animal: Failing - no differences, yes must change violations
CropAndSoil: Passing
Manure: Failing - yes differences, no must change violations
Feed: Passing
Not required obviously but just a thought for improving down the line.
There was a problem hiding this comment.
Also quick note that part of the reason I'm suggesting this is that these errors are getting more difficult to catch with the volume of logs and warnings that occur through all the simulations and now that an error can be multiple things, getting a summary of that at the end would be helpful. Or maybe we should think about changing the default verbosity of the e2e tests.
There was a problem hiding this comment.
I like this. I'd rather make it its own issue than add it to this PR, though. The useful version is one summary printed after all the runs finish, and since the sets run in parallel workers, that means the main process has to collect results from the tasks; that's complicated. This new issue's scope can include both the final reporting and the verbosity (As we discussed and brought up by Allister, it still needs to extract some log info related to the simulation status).
| if domain_must_change_variables: | ||
| comparison_results["must_change_satisfied"] = must_change_satisfied | ||
| comparison_results["must_change_violations"] = must_change_violations |
There was a problem hiding this comment.
I feel like it could be helpful to have the amount the variable changed by for reference reported here - maybe the change is a small fraction below or above the threshold and that would be helpful information to the SME. Or maybe it's must-change but the factor by which it changed is greatly out of proportion with the SME's expected change.
There was a problem hiding this comment.
I would say this is a lot more complicated than it seems. One main problem I can think of is that most flagged variables are a year-long series (feed 202's balance was 13 values; daily variables are 365) or lists of dictionaries. What "the amount" will be is hard to define. For example, what's the biggest percent change? The count of days beyond tolerance? The full diff? etc.
So this is something that I think might not be worth the hassle, since with such complexity in the outputs, I'm not sure if it's going to be that helpful in the end.
|
Current Coverage: 99% Mypy errors on e2e-must-change branch: 1133 |
3 participants
Adds "must change" support to end-to-end testing: an SME can flag variables that are expected to differ from the recorded expected results, and E2E now fails if they don't.
Context
Issue(s) closed by this pull request: closes #3158
What
*_must_change_variables.jsonfile (ininput/data/end_to_end_testing/) listing variables that must differ from the recorded expected results.changed_variables, a compiled list of the differing variable names, so an SME can evaluate them and flag the legitimate ones.Why
E2E filters are generated at version
X; when a model change lands, every intended difference inX+shows up as a failure. An SME often knows a variable must be different without knowing its new value, so this is a binary flag, not a new expected value. Thechanged_variableslist gives the SME a clean list to review instead of raw diff paths.How
must_change_variables_pathkey on every entry inend_to_end_testing_result_paths.jsonpoints to the input set's file (schema added inproperties/default.json).E2ETestResultsHandler, flagged variables are excluded from the regular DeepDiff and each one is checked individually with the same DeepDiff settings and tolerance filtering; outcomes are recorded asmust_change_satisfied/must_change_violations, andend_to_end_testing_passingrequires no regular differences and no violations.Test plan
animals_onlye2e metadata throughInputManager, synthesized actual results from the filter file, and rancompare_actual_and_expected_test_resultsfor all five scenarios above.dev).Complete workflow tests:
Flag something that doesn't change → violation. Change nothing else; add to the flag file: "AnimalModuleReporter.report_animal_population_statistics.population_number_of_cows". Run. Animal domain fails; the results file shows must_change_violations with "value still matches the expected results within the tolerance".
Change an input, flag nothing → the SME review list. Empty the flag list; in the animal file set animal_config.management_decisions.milk_fat_percent from 4 to 4.5. Run. Animal fails, and Animal.changed_variables in the results file is the compiled name list an SME would review.
Declare the change → pass. Keep the 4.5 tweak; copy the changed_variables array from scenario 2's results file straight into must_change_variables (same JSON shape on purpose). Run. Animal passes, with everything under must_change_satisfied. This is the intended workflow end to end.
Typo protection. Add "AnimalModuleReporter.not_a_real_variable" to the flag list. Run. You get the configuration error "Must-change variables not found in the expected results of any domain".
Input Changes
freestall/open_lot/field_and_feed/animals_only_must_change_variables.jsonfiles (empty lists by default).must_change_variables_pathto every entry inend_to_end_testing_result_paths.json, with matching schema inRUFAS/input/metadata/properties/default.json.Output Changes
{Domain}.changed_variables: names of unflagged variables whose values differ from the expected results.{Domain}.must_change_satisfied: flagged variables that did change.{Domain}.must_change_violations: flagged variables that did not change (or are missing), with the reason.Filter
No new filter needed — the existing
e2e_comparison_*filters ({Domain}.*patterns) already capture the new keys.