Is your feature request related to a problem?
Follow-up from #60, where @janhoy suggested this as a separate improvement:
It would of course be elegant if we auto resolved the workload location from the stored run. Perhaps as a separate improvement in addition to this more explicit one?
Today aggregate needs to be told where the workload lives (--workload-repository, or --workload-path after #60), even though it already knows which workload the runs used. The aggregator reads the workload name from the stored test run, then loads the workload spec to walk its schedule — so the name comes from the run but the location comes from the command line.
Describe the solution you'd like
aggregate resolves the workload source from the stored test run, so neither flag is needed in the common case. The flags stay as an override for the case where the workload has since moved.
Additional context — what this actually requires
Worth stating up front, because it is more than reading an existing field. The stored test run keeps the workload name, params and revision, but not where it came from:
TestRun.as_dict() persists workload, workload-params and workload-revision (metrics.py), and from_dict() restores those three.- Neither the repository name nor the workload path is persisted, so there is currently nothing in
test_run.json that says whether the run used the default repository, a different repository, or a local directory.
So the work is roughly:
- Persist the workload source in
TestRun.as_dict() / restore it in from_dict().
- Have the aggregator prefer the stored source, falling back to the flags.
- Handle test runs written before the change, which will not carry the field. There is existing precedent for that kind of grace period in
metrics.py (# TODO: cluster is optional for BWC. This can be removed after some grace period.).
Two things that probably need a decision before implementation:
I can work on it once there is a view on those two points — mainly whether the new field should be optional-with-fallback indefinitely or on a deprecation clock, and whether it is worth raising with OSB first.
Is your feature request related to a problem?
Follow-up from #60, where @janhoy suggested this as a separate improvement:
Today
aggregateneeds to be told where the workload lives (--workload-repository, or--workload-pathafter #60), even though it already knows which workload the runs used. The aggregator reads the workload name from the stored test run, then loads the workload spec to walk its schedule — so the name comes from the run but the location comes from the command line.Describe the solution you'd like
aggregateresolves the workload source from the stored test run, so neither flag is needed in the common case. The flags stay as an override for the case where the workload has since moved.Additional context — what this actually requires
Worth stating up front, because it is more than reading an existing field. The stored test run keeps the workload name, params and revision, but not where it came from:
TestRun.as_dict()persistsworkload,workload-paramsandworkload-revision(metrics.py), andfrom_dict()restores those three.test_run.jsonthat says whether the run used the default repository, a different repository, or a local directory.So the work is roughly:
TestRun.as_dict()/ restore it infrom_dict().metrics.py(# TODO: cluster is optional for BWC. This can be removed after some grace period.).Two things that probably need a decision before implementation:
test_run.jsonaffects files users already have; the fallback behaviour matters more than the happy path.aggregator.pyand the relevant part ofmetrics.pyare effectively identical to OSB's, so a format change here is worth coordinating rather than diverging on. (The threeaggregatefixes were reported in both projects for the same reason — aggregate has no --workload-path, so runs made with --workload-path can't be aggregated opensearch-project/opensearch-benchmark#1095 is the counterpart of aggregate has no --workload-path, so runs made with --workload-path can't be aggregated #57.)I can work on it once there is a view on those two points — mainly whether the new field should be optional-with-fallback indefinitely or on a deprecation clock, and whether it is worth raising with OSB first.