New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[qa] pull-tester: Run rpc test in parallel #7972

Merged
merged 2 commits into from May 10, 2016

Conversation

Projects
None yet
7 participants

@MarcoFalke MarcoFalke added the Tests label Apr 29, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 29, 2016

Member

Looks like this runs the extended tests (no pruning) 4 times faster on my machine:

Total time: 314 s (accumulated: 1306 s)
Member

MarcoFalke commented Apr 29, 2016

Looks like this runs the extended tests (no pruning) 4 times faster on my machine:

Total time: 314 s (accumulated: 1306 s)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 29, 2016

Member

Wow. This is a great idea.
Concept ACK.

Member

jonasschnelli commented Apr 29, 2016

Wow. This is a great idea.
Concept ACK.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 29, 2016

Member

Concept ACK, thanks for working on this!

Member

theuni commented Apr 29, 2016

Concept ACK, thanks for working on this!

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 29, 2016

Member

Nice! Concept ACK. I wil review better and benchmark another day.

Member

jtimon commented Apr 29, 2016

Nice! Concept ACK. I wil review better and benchmark another day.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake May 5, 2016

Member

First run testing this (shortened tests),

ALL   | True   | 764 s (accumulated)

Runtime: 212 s

Concept ACK

Member

fanquake commented May 5, 2016

First run testing this (shortened tests),

ALL   | True   | 764 s (accumulated)

Runtime: 212 s

Concept ACK

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 5, 2016

Member

@MarcoFalke This is great, thanks for tackling! Any idea what's causing the travis failure?

I think it would be nice to make the number of simultaneous jobs configurable on the command line; the default of 4 seems good but being able to tweak it for the machine it's running on (say if other developers run this script locally, as I do) would be nice.

I also noticed that by re-sorting the standard tests so that the slowest jobs start first, I was able to get the runtime of all the standard tests running in parallel down to the same as running the single longest test (walletbackup.py) -- awesome!

Member

sdaftuar commented May 5, 2016

@MarcoFalke This is great, thanks for tackling! Any idea what's causing the travis failure?

I think it would be nice to make the number of simultaneous jobs configurable on the command line; the default of 4 seems good but being able to tweak it for the machine it's running on (say if other developers run this script locally, as I do) would be nice.

I also noticed that by re-sorting the standard tests so that the slowest jobs start first, I was able to get the runtime of all the standard tests running in parallel down to the same as running the single longest test (walletbackup.py) -- awesome!

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 5, 2016

Member

Hm, I just ran the extended tests and got an odd failure -- this test looks like it is supposed to be marked successful.

bip9-softforks.py:
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Test 1: PASS [143]
Test 2: PASS [287]
Test 3: PASS [431]
Test 4: PASS [574]
Test 5: PASS [575]
Test 6: PASS [575]
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Test 7: PASS [143]
Test 8: PASS [287]
Test 9: PASS [431]
Test 10: PASS [574]
Test 11: PASS [575]
Test 12: PASS [575]
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Test 13: PASS [143]
Test 14: PASS [287]
Test 15: PASS [431]
Test 16: PASS [574]
Test 17: PASS [575]
Test 18: PASS [575]
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Stopping nodes
Cleaning up
Tests successful

BDB3028 /tmp/testE9D_TH/blocks.db: unable to flush: No such file or directory

Pass: False, Duration: 41 s

Maybe that last line about "unable to flush" is going to stderr, causing this to be recorded as failure? If so then probably the test should be fixed.

Member

sdaftuar commented May 5, 2016

Hm, I just ran the extended tests and got an odd failure -- this test looks like it is supposed to be marked successful.

bip9-softforks.py:
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Test 1: PASS [143]
Test 2: PASS [287]
Test 3: PASS [431]
Test 4: PASS [574]
Test 5: PASS [575]
Test 6: PASS [575]
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Test 7: PASS [143]
Test 8: PASS [287]
Test 9: PASS [431]
Test 10: PASS [574]
Test 11: PASS [575]
Test 12: PASS [575]
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Test 13: PASS [143]
Test 14: PASS [287]
Test 15: PASS [431]
Test 16: PASS [574]
Test 17: PASS [575]
Test 18: PASS [575]
Initializing test directory /tmp/testE9D_TH
MiniNode: Connecting to Bitcoin Node IP # 127.0.0.1:13641
Stopping nodes
Cleaning up
Tests successful

BDB3028 /tmp/testE9D_TH/blocks.db: unable to flush: No such file or directory

Pass: False, Duration: 41 s

Maybe that last line about "unable to flush" is going to stderr, causing this to be recorded as failure? If so then probably the test should be fixed.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake May 6, 2016

Member

Agree with @sdaftuar that there is benefit in sorting the standard tests.
Longest -> shortest:

ALL | True   | 857 s (accumulated)
Runtime: 218 s

Compared to as is:

ALL | True   | 845 s (accumulated)
Runtime: 238 s
Member

fanquake commented May 6, 2016

Agree with @sdaftuar that there is benefit in sorting the standard tests.
Longest -> shortest:

ALL | True   | 857 s (accumulated)
Runtime: 218 s

Compared to as is:

ALL | True   | 845 s (accumulated)
Runtime: 238 s
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 6, 2016

Contributor

ACK fa48ef8

Thanks!

Edit: I was checking it pre-python3, so ...

Contributor

paveljanik commented May 6, 2016

ACK fa48ef8

Thanks!

Edit: I was checking it pre-python3, so ...

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake May 6, 2016

Member

This needs to be updated to use http.client before it can merged, now that the Python3 change is in.

Member

fanquake commented May 6, 2016

This needs to be updated to use http.client before it can merged, now that the Python3 change is in.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 9, 2016

Member
  • Rebased and translated to python3
  • made it possible to specify -parallel=n on the command line
  • Moved walletbackup.py to first position
  • --portseed is set in a deterministic manner for each rpc test
Member

MarcoFalke commented May 9, 2016

  • Rebased and translated to python3
  • made it possible to specify -parallel=n on the command line
  • Moved walletbackup.py to first position
  • --portseed is set in a deterministic manner for each rpc test
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 10, 2016

Member
$ python3 ./qa/pull-tester/rpc-tests.py -parallel=20000
...
ALL                            | True   | 634 s (accumulated)
Runtime: 143 s

$ python3 ./qa/pull-tester/rpc-tests.py -parallel=20000 -extended
...
maxuploadtarget.py             | True   | 81 s
walletbackup.py                | True   | 135 s
smartfees.py                   | True   | 341 s
pruning.py                     | True   | 1503 s

ALL                            | False  | 3076 s (accumulated)

Runtime: 1503 s

Now I'm too lazzy to run them before this patch to compare...benchmark conclusion: this is great.

It also comes to mind that, in terms of time, only smartfees and pruning deserve to be in the extended tests group more then walletbackup. But probably moving walletbackup to the extended tests (and maybe some of the faster ones out of the extended group to the regular one at the same time?) is out of the scope of this PR.

Tested ACK.

Member

jtimon commented May 10, 2016

$ python3 ./qa/pull-tester/rpc-tests.py -parallel=20000
...
ALL                            | True   | 634 s (accumulated)
Runtime: 143 s

$ python3 ./qa/pull-tester/rpc-tests.py -parallel=20000 -extended
...
maxuploadtarget.py             | True   | 81 s
walletbackup.py                | True   | 135 s
smartfees.py                   | True   | 341 s
pruning.py                     | True   | 1503 s

ALL                            | False  | 3076 s (accumulated)

Runtime: 1503 s

Now I'm too lazzy to run them before this patch to compare...benchmark conclusion: this is great.

It also comes to mind that, in terms of time, only smartfees and pruning deserve to be in the extended tests group more then walletbackup. But probably moving walletbackup to the extended tests (and maybe some of the faster ones out of the extended group to the regular one at the same time?) is out of the scope of this PR.

Tested ACK.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 10, 2016

Member

This is great. Looking at a random travis before/after, this goes from 535sec to 164sec.
untested (locally) ACK.

Member

theuni commented May 10, 2016

This is great. Looking at a random travis before/after, this goes from 535sec to 164sec.
untested (locally) ACK.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 10, 2016

Member

This looks great, though there's still the issue with the bip9-softforks.py test outputting to stderr, causing the extended tests to fail. I'm trying to figure out a fix to suggest for that, but I'm not having any great ideas at the moment...

Member

sdaftuar commented May 10, 2016

This looks great, though there's still the issue with the bip9-softforks.py test outputting to stderr, causing the extended tests to fail. I'm trying to figure out a fix to suggest for that, but I'm not having any great ideas at the moment...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 10, 2016

Contributor

reACK ccccc59

Contributor

paveljanik commented May 10, 2016

reACK ccccc59

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 10, 2016

Member

@sdaftuar This seems to be a local problem or a problem with the bip9 test and is not caused by this pull, I assume?

Runs fine locally, just now:

TEST              | PASSED | DURATION

bip9-softforks.py | True   | 66 s

ALL               | True   | 66 s (accumulated)

Runtime: 66 s
Member

MarcoFalke commented May 10, 2016

@sdaftuar This seems to be a local problem or a problem with the bip9 test and is not caused by this pull, I assume?

Runs fine locally, just now:

TEST              | PASSED | DURATION

bip9-softforks.py | True   | 66 s

ALL               | True   | 66 s (accumulated)

Runtime: 66 s
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 10, 2016

Member

Yep I didn't realize this was just affecting me, will open a separate issue.

ACK.

Member

sdaftuar commented May 10, 2016

Yep I didn't realize this was just affecting me, will open a separate issue.

ACK.

@MarcoFalke MarcoFalke merged commit ccccc59 into bitcoin:master May 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request May 10, 2016

Merge #7972: [qa] pull-tester: Run rpc test in parallel
ccccc59 [qa] Add option --portseed to test_framework (MarcoFalke)
fa494de [qa] pull-tester: Run rpc test in parallel (MarcoFalke)

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1604-qaParallel branch May 10, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7972: [qa] pull-tester: Run rpc test in parallel
ccccc59 [qa] Add option --portseed to test_framework (MarcoFalke)
fa494de [qa] pull-tester: Run rpc test in parallel (MarcoFalke)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7972: [qa] pull-tester: Run rpc test in parallel
ccccc59 [qa] Add option --portseed to test_framework (MarcoFalke)
fa494de [qa] pull-tester: Run rpc test in parallel (MarcoFalke)

codablock added a commit to codablock/dash that referenced this pull request Dec 21, 2017

Merge #7972: [qa] pull-tester: Run rpc test in parallel
ccccc59 [qa] Add option --portseed to test_framework (MarcoFalke)
fa494de [qa] pull-tester: Run rpc test in parallel (MarcoFalke)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment