You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org> on 2026/07/25 15:02:18 UTC
[PR] Add --workload-path to aggregate, for parity with run [solr-orbit]
serhiy-bzhezytskyy opened a new pull request, #60:
URL: https://github.com/apache/solr-orbit/pull/60
### Description
`add_workload_source()` offers a workload either as a repository or as a local path, and `run`, `list` and
`info` all use it. `aggregate` defined only its own `--workload-repository`, so a workload developed
locally and benchmarked with `run --workload-path` could not be aggregated afterwards:
```
solr-orbit: error: unrecognized arguments: --workload-path=/path/to/my-workload
```
`aggregate` now uses the same helper, so it accepts either source, keeps the two mutually exclusive, and
gains `--workload-revision` for free. It also goes through `configure_workload_params`, so a path is
normalised and validated the way it is everywhere else — that is also what rejects `--workload-revision`
together with `--workload-path`. `command_requires_workload=False` because `aggregate` takes the workload
name from the stored test run, not from the command line.
One behavioural detail: a path sets `workload.path`, which is what makes the loader choose
`SimpleWorkloadRepository`. So when a path was given, `aggregate()` no longer names a repository — doing
both would send the loader looking for the workload inside that repository instead. That guard is
load-bearing, since argparse's default for `--workload-repository` is the string `"default"`, not `None`.
### Issues Resolved
Resolves #57
### Testing
- [x] New functionality includes testing
- Two new tests: `test_aggregate_names_the_workload_repository` (unchanged behaviour when a repository is
used) and `test_aggregate_leaves_a_workload_path_alone` (no `repository.name` is configured when a path
is given).
- The shared `mock_args` fixture previously supplied no `workload_path`, so the aggregator saw a `Mock`
attribute that happens to be truthy. It is now explicit — that fixture change is what makes the first
test meaningful rather than accidentally passing.
- CLI verified by hand on this branch: `aggregate --workload-path=...` is accepted where it previously
errored, and combining it with `--workload-revision` is correctly rejected.
- Full suite: `1100 passed, 5 skipped`, plus one unrelated pre-existing flake described below.
`ruff check` clean.
On the flake, so it isn't mistaken for fallout from this PR:
`tests/workload/workload_test.py::DocumentCorpusTests::test_union_document_corpora_is_symmetric` fails on
roughly one interpreter start in eight. `DocumentCorpus.union` builds its document list with
`list(set(...).union(...))` while `DocumentCorpus.__eq__` compares that list in order, so symmetry depends
on hash iteration order. It reproduces deterministically under `PYTHONHASHSEED=7` on `main`'s untouched
workload code and on the other two fix branches, and this branch does not touch `solrorbit/workload/` at
all. I'll report it separately rather than fold an unrelated fix in here.
### Notes
Found while running a multi-configuration geonames campaign comparing Solr on Lucene 10.4 against
Lucene 11. Third of three `aggregate` gaps, and the only one that is a parity gap rather than a bug.
There is a case for going further — a test run already records the workload it used, so `aggregate` could
resolve the spec from the stored run and need no workload-source argument at all. I've gone with the
parity fix because it is the smaller change and matches the existing pattern, but I'm open to the other
shape if you'd rather have it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add --workload-path to aggregate, for parity with run [solr-orbit]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on PR #60:
URL: https://github.com/apache/solr-orbit/pull/60#issuecomment-5092820829
Thanks Jan.
On auto-resolving the workload from the stored test run — agreed, a separate improvement. This PR keeps to the parity fix (`run` accepts a workload source, `aggregate` should too); I can open a follow-up issue for the auto-resolve idea so it isn't lost.
On the docs: you're right, and it's slightly worse than it looks. The two pages disagreed with each other — `command-flags.md` already listed `--workload-repository` under "aggregate flags", while `reference/commands/aggregate.md` documented only `--test-runs`, `--test-runs-id` and `--results-file`. Neither page mentioned `--workload-revision`, which `aggregate` has accepted all along, since it calls the same `add_workload_source()` helper as `run`.
Pushed a commit documenting all three on both pages, using the wording already used for `run`. `--workload-path` and `--workload-repository` are mutually exclusive (argparse group), so they read the same way there as elsewhere.
Worth noting this doc gap is probably why the parity gap went unnoticed: with `--workload-repository` undocumented on that page, the absence of `--workload-path` beside it wasn't visible to a reader.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add --workload-path to aggregate, for parity with run [solr-orbit]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on PR #60:
URL: https://github.com/apache/solr-orbit/pull/60#issuecomment-5093779738
Filed the auto-resolve follow-up as #61 so it isn't lost.
One thing worth flagging that came out of writing it up: the stored test run doesn't currently record *where* the workload came from. `TestRun.as_dict()` persists the workload name, `workload-params` and `workload-revision`, and `from_dict()` restores those three — but nothing says whether the run used the default repository, another repository, or a local path. So auto-resolving would need that field persisted first, plus a fallback for test-run files written before the change (there's precedent for that grace period in `metrics.py`). The issue spells that out, along with the two decisions that seem worth settling before any code: whether the field is optional-with-fallback indefinitely or on a deprecation clock, and whether it's worth coordinating with OpenSearch Benchmark, since `aggregator.py` there is the same file apart from the import.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add --workload-path to aggregate, for parity with run [solr-orbit]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy merged PR #60:
URL: https://github.com/apache/solr-orbit/pull/60
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org