You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/03/29 22:47:39 UTC

[GitHub] [arrow] dianaclarke opened a new pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

dianaclarke opened a new pull request #9843:
URL: https://github.com/apache/arrow/pull/9843


   …_json


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r604055934



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order

Review comment:
       Starting from Python 3.7, dicts preserve insertion order:
   file:///home/antoine/cpython/default/Doc/build/html/whatsnew/3.7.html
   




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#issuecomment-810010536


   https://issues.apache.org/jira/browse/ARROW-12145


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] cyb70289 commented on pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
cyb70289 commented on pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#issuecomment-810690610


   Thanks @dianaclarke


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] cyb70289 commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r603794401



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order
+    artificial, unchanged = comparisons[0], comparisons[1]
+    if comparisons[0].name == "FloatParsing<FloatType>":
+        artificial, unchanged = comparisons[1], comparisons[0]

Review 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.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r604396615



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order

Review comment:
       I see, thanks.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] cyb70289 commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r603794401



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order
+    artificial, unchanged = comparisons[0], comparisons[1]
+    if comparisons[0].name == "FloatParsing<FloatType>":
+        artificial, unchanged = comparisons[1], comparisons[0]

Review comment:
       This code is a bit strange, looks like workaround to a bug.
   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.

##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order

Review 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.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] dianaclarke commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
dianaclarke commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r604325602



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order

Review 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])
   ```




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r604055934



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)
+
+    # can't assume return order

Review comment:
       Starting from Python 3.7, dicts preserve insertion order:
   https://docs.python.org/3.9/whatsnew/3.7.html
   
   




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] dianaclarke commented on pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
dianaclarke commented on pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#issuecomment-810257518


   Thanks for the reviews, folks.
   
   I decided to just split the test in two:
   
   - `test_static_runner_from_json_not_a_regression`
   - `test_static_runner_from_json_regression`


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] cyb70289 closed pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
cyb70289 closed pull request #9843:
URL: https://github.com/apache/arrow/pull/9843


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] pitrou commented on a change in pull request #9843: ARROW-12145: [Developer][Archery] Flaky: test_static_runner_from_json

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #9843:
URL: https://github.com/apache/arrow/pull/9843#discussion_r604057856



##########
File path: dev/archery/archery/tests/test_benchmarks.py
##########
@@ -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)

Review 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.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org