serhiy-bzhezytskyy opened a new pull request, #58:
URL: https://github.com/apache/solr-orbit/pull/58
### Description
`calculate_weighted_average` reduced `min`/`max` with
`value.get(metric_field, 0)`. That default only
applies when the key is *absent*, so a key that is present with value `None`
reached `min()`/`max()` and
raised `TypeError: '<' not supported between instances of 'NoneType' and
'NoneType'`. The
percentile/median branch just below had the same flaw in a different shape —
`value * iterations` on
`None`.
`calculate_rsd` is a second, independent site, reached from
`build_aggregated_results_dict` with the
per-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.
`optimize` does this in the
geonames workload, and `optimize` is part of the default workload, so every
aggregation of a geonames
campaign failed.
The change:
- extracts the weighted mean into a `weighted_mean` helper, since the dict
and scalar branches were
computing it two different ways and both needed the same treatment;
- leaves nulls out of both the weighted sum and its divisor, so a run that
measured nothing does not
drag the mean toward zero;
- propagates `None` when no run contributed a value, rather than
substituting `0` — `0` would read as
"throughput was zero", which is a different claim from "not measured".
This matches how
`aggregate_json_by_key` in the same file already treats nulls;
- returns `NA` from `calculate_rsd` when no values remain, as it already
does for the single-value case.
### Issues Resolved
Resolves #55
### Testing
- [x] New functionality includes testing
- Two new tests in `tests/aggregator_test.py`: all-null metric fields (the
`optimize` case), and
partially 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 restoring `main`'s
`aggregator.py` under the new
tests, not by inspection.
- Full suite: `1101 passed, 5 skipped`. `ruff check` clean.
- 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
`optimize` reported as
`{"mean": null, "mean_rsd": "NA"}` instead of a fabricated zero.
The `calculate_rsd` site 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 — `aggregate` computes the weighted means and per-metric RSD that
make a multi-run result
readable, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
