ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json #9843
Conversation
| artificial_reg, normal = RunnerComparator(contender, baseline).comparisons | ||
| comparisons = list(RunnerComparator(contender, baseline).comparisons) | ||
|
|
||
| # can't assume return order |
There was a problem hiding this comment.
The return order is not determined as internally, benchmarks list is changed to pythons dict which is unordered.
https://github.com/apache/arrow/blob/master/dev/archery/archery/benchmark/compare.py#L140-L145
Result of the first benchmark may return after the result of the second benchmark.
There was a problem hiding this comment.
Starting from Python 3.7, dicts preserve insertion order:
https://docs.python.org/3.9/whatsnew/3.7.html
There was a problem hiding this comment.
While dictionary order is preserved in Python 3, set order isn't (to the best of my knowledge).
>>> foo = {'a': 1, 'b': 2, 'c': 3}
>>> bar = {'a': 1, 'b': 2, 'c': 3}
>>> type(foo.keys() & bar.keys())
<class 'set'>
I suspect it's the set usage in here, that makes the resulting order non-deterministic.
def pairwise_compare(contender, baseline):
dict_contender = {e.name: e for e in contender}
dict_baseline = {e.name: e for e in baseline}
for name in (dict_contender.keys() & dict_baseline.keys()): # <------ for name in set
yield name, (dict_contender[name], dict_baseline[name])
| artificial, unchanged = comparisons[0], comparisons[1] | ||
| if comparisons[0].name == "FloatParsing<FloatType>": | ||
| artificial, unchanged = comparisons[1], comparisons[0] |
There was a problem hiding this comment.
This code is a bit strange and hard to reason.
I would recommend changing the test suite to contain only one benchmark.
First test the non-regression case. Then introduce artificial regression and test the regression case.
| @@ -94,10 +94,16 @@ def test_static_runner_from_json(): | |||
| archery_result['suites'][0]['benchmarks'][0]['values'][0] *= 2 | |||
| baseline = StaticBenchmarkRunner.from_json(json.dumps(archery_result)) | |||
|
|
|||
| artificial_reg, normal = RunnerComparator(contender, baseline).comparisons | |||
| comparisons = list(RunnerComparator(contender, baseline).comparisons) | |||
There was a problem hiding this comment.
Why not explicitly sort the results? Assuming the benchmark name is available somewhere in the comparisons objects, this would allow making the ordering deterministic.
|
Thanks for the reviews, folks. I decided to just split the test in two:
|
|
Thanks @dianaclarke |
This test assumes: ``` artificial_reg, normal = RunnerComparator(contender, baseline).comparisons ``` When the return order could be: ``` normal, artificial_reg = RunnerComparator(contender, baseline).comparisons ``` The return order of `comparisons` isn't deterministic. See: https://issues.apache.org/jira/browse/ARROW-12145 Closes apache#9843 from dianaclarke/ARROW-12145 Authored-by: Diana Clarke <diana.joan.clarke@gmail.com> Signed-off-by: Yibo Cai <yibo.cai@arm.com>
This test assumes: ``` artificial_reg, normal = RunnerComparator(contender, baseline).comparisons ``` When the return order could be: ``` normal, artificial_reg = RunnerComparator(contender, baseline).comparisons ``` The return order of `comparisons` isn't deterministic. See: https://issues.apache.org/jira/browse/ARROW-12145 Closes apache#9843 from dianaclarke/ARROW-12145 Authored-by: Diana Clarke <diana.joan.clarke@gmail.com> Signed-off-by: Yibo Cai <yibo.cai@arm.com>
This test assumes: ``` artificial_reg, normal = RunnerComparator(contender, baseline).comparisons ``` When the return order could be: ``` normal, artificial_reg = RunnerComparator(contender, baseline).comparisons ``` The return order of `comparisons` isn't deterministic. See: https://issues.apache.org/jira/browse/ARROW-12145 Closes apache#9843 from dianaclarke/ARROW-12145 Authored-by: Diana Clarke <diana.joan.clarke@gmail.com> Signed-off-by: Yibo Cai <yibo.cai@arm.com>
This test assumes:
When the return order could be:
The return order of
comparisonsisn't deterministic.See: https://issues.apache.org/jira/browse/ARROW-12145