Fix: aggregate crashes when an operation reports null metrics - #58
Open
serhiy-bzhezytskyy wants to merge 2 commits into
Open
Fix: aggregate crashes when an operation reports null metrics#58serhiy-bzhezytskyy wants to merge 2 commits into
serhiy-bzhezytskyy wants to merge 2 commits into
Conversation
calculate_weighted_average reduced min/max with value.get(field, 0). That default only applies when the key is absent, so a key present with value None reached min()/max() and raised TypeError. The percentile/median branch had the same flaw: None * iterations. calculate_rsd was a second, independent site, reached from build_aggregated_results_dict via the per-run mean values. An operation that produced no valid samples reports its metrics as null -- optimize does this in the geonames workload, so every aggregation of a geonames campaign failed. Nulls are now left out of both the weighted sum and its divisor, and propagate as None rather than becoming a measured zero, matching how aggregate_json_by_key already treats them. RSD over no values reports NA, as it already does for a single value.
1 task
There was a problem hiding this comment.
Pull request overview
Fixes solr-orbit aggregate crashes when operations report null metric values (e.g., optimize in geonames) by excluding None from weighted aggregations and RSD calculations, and by propagating “not measured” as None/"NA" instead of fabricating zeros.
Changes:
- Add a
weighted_meanhelper and update weighted aggregation logic to ignoreNonevalues (including in dict metric fields likemin/max/median/percentiles). - Update
calculate_rsdto ignoreNoneinputs and return"NA"when no numeric values remain. - Add unit tests covering all-null and partially-null metric-field aggregation cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
solrorbit/aggregator.py |
Makes aggregation resilient to None metric samples via weighted_mean and calculate_rsd filtering. |
tests/aggregator_test.py |
Adds regression tests ensuring null metric fields don’t crash and aggregate correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return weighted_metrics | ||
|
|
||
| @staticmethod | ||
| def weighted_mean(values: List[Any], iterations_per_run: List[int]) -> Any: |
Comment on lines
255
to
+260
| def calculate_rsd(self, values: List[Union[int, float]], metric_name: str) -> Union[float, str]: | ||
| if not values: | ||
| raise ValueError(f"Cannot calculate RSD for metric '{metric_name}': empty list of values") | ||
| # operations that produced no valid samples report None, which cannot contribute to a deviation | ||
| values = [value for value in values if value is not None] | ||
| if not values: |
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
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.
Description
calculate_weighted_averagereducedmin/maxwithvalue.get(metric_field, 0). That default onlyapplies when the key is absent, so a key that is present with value
Nonereachedmin()/max()andraised
TypeError: '<' not supported between instances of 'NoneType' and 'NoneType'. Thepercentile/median branch just below had the same flaw in a different shape —
value * iterationsonNone.calculate_rsdis a second, independent site, reached frombuild_aggregated_results_dictwith theper-run mean values. Fixing only the first one moves the crash rather than removing it, which is how I
found the second: unit tests for the first site were already green while real data still failed.
An operation that produced no valid samples reports its metrics as null.
optimizedoes this in thegeonames workload, and
optimizeis part of the default workload, so every aggregation of a geonamescampaign failed.
The change:
weighted_meanhelper, since the dict and scalar branches werecomputing it two different ways and both needed the same treatment;
drag the mean toward zero;
Nonewhen no run contributed a value, rather than substituting0—0would read as"throughput was zero", which is a different claim from "not measured". This matches how
aggregate_json_by_keyin the same file already treats nulls;NAfromcalculate_rsdwhen no values remain, as it already does for the single-value case.Issues Resolved
Resolves #55
Testing
New functionality includes testing
Two new tests in
tests/aggregator_test.py: all-null metric fields (theoptimizecase), andpartially null — a metric with samples in one run but not another, where the valid values must still
aggregate, weighted only by the runs that contributed them.
Both fail on
main(TypeError: '<' not supported between instances of 'NoneType' and 'NoneType'and... 'NoneType' and 'int') and pass here. Verified by restoringmain'saggregator.pyunder the newtests, not by inspection.
Full suite:
1101 passed, 5 skipped.ruff checkclean.End-to-end on real data: aggregated 3 configurations × 5 runs of the full geonames corpus (11M docs).
Before, every one of those aggregations crashed; now they complete, with
optimizereported as{"mean": null, "mean_rsd": "NA"}instead of a fabricated zero.The
calculate_rsdsite has no dedicated unit test here — it is covered by the end-to-end runs above,where it was the crash that appeared once the first site was fixed. I can add a direct one if you'd
prefer it in the suite.
Notes
Found while running a multi-configuration geonames campaign to compare Solr on Lucene 10.4 against
Lucene 11 —
aggregatecomputes the weighted means and per-metric RSD that make a multi-run resultreadable, so this was a hard stop.
The same code is present in OpenSearch Benchmark, which this is a port of, so the defect looks inherited
rather than introduced here. I intend to report it upstream too.