impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4735: Upgrade pytest in python env to version 2.9.2.
Date Tue, 24 Jan 2017 04:14:13 GMT
David Knupp has posted comments on this change.

Change subject: IMPALA-4735: Upgrade pytest in python env to version 2.9.2.
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5640/3//COMMIT_MSG
Commit Message:

PS3, Line 7: 2.9.2
> Why not 3.0.5?
I was going to add a comment about this to the JIRA.

Essentially, I couldn't find the workaround/fix to get rid of this 3.0.5-specific warning,
and I thought an incremental upgrade would at least solve some of our issues:

   WP1 None Module already imported so can not be re-written: random_plugin
   WP1 None Module already imported so can not be re-written: xdist
   WP1 None Module already imported so can not be re-written: xdist
   WP1 None Module already imported so can not be re-written: xdist


Line 13: In addition to bumping the version of pytest (and related modules),
> It might make more sense to split these changes up.
I can do that if you feel strongly about it.


Line 15: showed up from pytest re: the imported TestMatrix and TestDimension
> Could you put in the commit message the sed script (or whatever) you used t
Done


Line 24: Tested by doing a standard (non-exhaustive) test run on centos 6.4
> Presumably that checks that the tests that run do not fail. How did you ver
OK, so this seemingly innocuous question opened up (what seems to me) a really weird can of
worms.

First, it is possible to to do a dry-run of sorts if you specify the --collect-only option.
This will show exactly which tests would have been executed, taking into consideration all
test vectors.

Since run-tests.py doesn't seem to honor --collect-only, you have to use impala-py.test directly,
which means you have to explicitly pass in the test directories that run-tests.py would have
typically white-listed out for you. E.g., this is the command for getting an exhaustive dry-run:

  $ impala-py.test --exploration_strategy=exhaustive --collect-only failure/ query_test/ stress/
aux_query_tests/ shell/ hs2/ catalog_service/ metadata/ data_errors/ statestore/ unittests/


To test, I ran it first from asf-gerrit-master branch (pytest v2.7.2), and then my dev branch
(pytest v2.9.2), each time blowing away the python virtual environment and bootstrapping a
new one. If I grep for "collected" in each file, the number was always the same -- 20881 items...

  $ grep collected ~/tmp/pytest_logs/pytest*exhaustive*log
  /home/dknupp/tmp/pytest_logs/pytest272_exhaustive_collect2.log:collecting ... collected
20881 items
  /home/dknupp/tmp/pytest_logs/pytest272_exhaustive_collect.log:collecting ... collected 20881
items
  /home/dknupp/tmp/pytest_logs/pytest292_exhaustive_collect2.log:collecting ... collected
20881 items
  /home/dknupp/tmp/pytest_logs/pytest292_exhaustive_collect.log:collecting ... collected 20881
items


However, I was surprised to learn that this number tended to vary when collecting tests with
the core exploration_strategy -- even when comparing successive runs on the same branch. I
narrowed it down to a single test -- query_test::test_cancellation::TestCancellationSerial::test_cancel_insert.

If I log output from the following command 10 times (regardless of version of pytest we use)
in quick succession:

  $ impala-py.test --exploration_strategy=core --collect-only query_test/test_cancellation.py


...and again grep for the collected items, the result varies between 51 and 57 items:

  $ grep collected ~/tmp/pytest_logs/pytest272_test_cancellation_collect*
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect0.log:collecting ... collected
51 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect1.log:collecting ... collected
52 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect2.log:collecting ... collected
53 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect3.log:collecting ... collected
54 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect4.log:collecting ... collected
53 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect5.log:collecting ... collected
55 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect6.log:collecting ... collected
53 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect7.log:collecting ... collected
57 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect8.log:collecting ... collected
53 items
  /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect9.log:collecting ... collected
52 items


Compare this to the result if we try another test module. In this case, the result is consistent
every time:

  $ grep collected ~/tmp/pytest_logs/pytest292_test_queries_collect*
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect0.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect1.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect2.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect3.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect4.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect5.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect6.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect7.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect8.log:collecting ... collected
38 items
  /home/dknupp/tmp/pytest_logs/pytest292_test_queries_collect9.log:collecting ... collected
38 items


If we cat the output of two of the log files, we can see that from run to run, it's actually
using different vectors:

  $ cat /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect6.log | grep test_cancel_insert
      <Function "test_cancel_insert[table_format: text/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: CTAS | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
select count(l_returnflag) pk from lineitem]">
      <Function "test_cancel_insert[table_format: seq/gzip/block | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: parquet/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: CTAS | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
select count(l_returnflag) pk from lineitem]">
      <Function "test_cancel_insert[table_format: text/gzip/block | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: parquet/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">


In the run immediately following, it uses:

  $ cat /home/dknupp/tmp/pytest_logs/pytest272_test_cancellation_collect7.log | grep test_cancel_insert
      <Function "test_cancel_insert[table_format: avro/snap/block | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: text/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: CTAS | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
select l_returnflag from lineitem]">
      <Function "test_cancel_insert[table_format: seq/snap/block | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: avro/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: seq/gzip/block | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: text/gzip/block | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: parquet/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: CTAS | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
select count(l_returnflag) pk from lineitem]">
      <Function "test_cancel_insert[table_format: parquet/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: SELECT | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
compute stats lineitem]">
      <Function "test_cancel_insert[table_format: text/none | exec_option: {'disable_codegen':
False, 'abort_on_error': 1, 'exec_single_node_rows_threshold': 0, 'batch_size': 0, 'num_nodes':
0} | query_type: CTAS | max_block_mgr_memory: 0 | cancel_delay: 3 | action: None | query:
select count(l_returnflag) pk from lineitem]">


I also find that actually running the tests, whether or not with run-tests.py or pytest directly,
produces different results for this one test (although the same number of PASSED tests is
always reported as 48.)

  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest272_runtests_tests0.log | wc -l
  6
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest272_runtests_tests1.log | wc -l
  6
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest272_runtests_tests2.log | wc -l
  8
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest272_runtests_tests3.log | wc -l
  9
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest272_runtests_tests4.log | wc -l
  7

  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest292_runtests_tests0.log | wc -l
  3
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest292_runtests_tests1.log | wc -l
  6
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest292_runtests_tests2.log | wc -l
  6
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest292_runtests_tests3.log | wc -l
  2
  $ grep test_cancel_insert ~/tmp/pytest_logs/pytest292_runtests_tests4.log | wc -l
  6


Kinda not sure what to make of this yet.


http://gerrit.cloudera.org:8080/#/c/5640/3/tests/run-tests.py
File tests/run-tests.py:

PS3, Line 101: we need to account for
> Why? What bad thing could happen otherwise?
I moved this line, and I think the context was lost. I've edited the comment to hopefully
be more clear. Essentially though, this is specifically required by IMPALA-4510 (and generally
related to the contortions stemming from IMPALA-4417.)

The essential reason is that at various stages of our test run, we need to alternately filter
and/or augment the list of pytest args. If we're filtering args, we need to make sure we don't
filter away ones that we need. If we separate them, we can do this by checking each arg against
the pytest config values, e.g.:

this will be retained:

   (Pdb) pytest.config.getvalue('metastore_server')
   'localhost:9083'
   

but this would be (incorrectly) filtered out:

   (Pdb) pytest.config.getvalue('metastore_server=localhost:9083')
   *** ValueError: no option named 'metastore_server=localhost:9083'

I agree that this is all regrettably ugly.


PS3, Line 103: random
> "arbitrary", not "random"
Done


PS3, Line 143: Because of the way our repo is organized.
> This doesn't explain a lot as it stands. I'm also not sure if it is a conti
Thanks. Hopefully the amended version is more clear.


-- 
To view, visit http://gerrit.cloudera.org:8080/5640
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I40d129e0e63ca5bee126bac6ac923abb3c7e0a67
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message